UBIFS assert failed in ubifs_dirty_inode

Submitted by Jeff Angielski on Jan. 27, 2010, 1:44 a.m.

Details

Message ID 4B5F9A78.7080000@theptrgroup.com
State New, archived
Headers show

Commit Message

Jeff Angielski Jan. 27, 2010, 1:44 a.m.
Artem Bityutskiy wrote:
> On Mon, 2010-01-25 at 23:48 -0600, Matt Mackall wrote:
>> Hmm. I'd just as soon drop it entirely. Here's a patch. Herbert, you
>> want to send this through your crypto tree?
>>
>>
>> random: drop weird m_time/a_time manipulation
>>
>> No other driver does anything remotely like this that I know of except
>> for the tty drivers, and I can't see any reason for random/urandom to do
>> it. In fact, it's a (trivial, harmless) timing information leak. And
>> obviously, it generates power- and flash-cycle wasting I/O, especially
>> if combined with something like hwrngd. Also, it breaks ubifs's
>> expectations.
>>
>> Signed-off-by: Matt Mackall <mpm@selenic.com>
>>
>> diff -r 29db0c391ce8 drivers/char/random.c
>> --- a/drivers/char/random.c	Sun Jan 17 11:01:16 2010 -0800
>> +++ b/drivers/char/random.c	Mon Jan 25 23:32:00 2010 -0600
>> @@ -1051,12 +1051,6 @@
>>  				/* like a named pipe */
>>  	}
>>  
>> -	/*
>> -	 * If we gave the user some bytes, update the access time.
>> -	 */
>> -	if (count)
>> -		file_accessed(file);
>> -
>>  	return (count ? count : retval);
>>  }
>>  
>> @@ -1116,8 +1110,6 @@
>>  	if (ret)
>>  		return ret;
>>  
>> -	inode->i_mtime = current_fs_time(inode->i_sb);
>> -	mark_inode_dirty(inode);
>>  	return (ssize_t)count;
>>  }
> 
> It may brake other FSes expectations, theoretically, as well.
> 
> Anyway, I'm perfectly fine if this is removed.
> 
> Jeff, could you please try Matt's patch and report back if you still
> have issues or not. If no, you can use this as a temporary work-around
> until a proper fix hits upstream or ubifs-2.6.git.

Matt's patch did not compile as written.  I tried to implement what I 
think he was trying to do and created this patch (it seems to match the 
guts of what inode_setattr() was looking for):


  	if (ret)
@@ -1116,8 +1117,12 @@ static ssize_t random_write(struct file *file, 
const char __user *buffer,
  	if (ret)
  		return ret;

-	inode->i_mtime = current_fs_time(inode->i_sb);
-	mark_inode_dirty(inode);
+	attr.ia_mtime = current_fs_time(inode->i_sb);
+	attr.ia_valid = ATTR_MTIME;
+	ret = inode_setattr(inode, &attr);
+	if (ret)
+		return ret;
+
  	return (ssize_t)count;
  }

However, this patch does not fix the problem.  I still see the same 
errors.  Matt, is this what you were trying to do?

I've also included the console dump just in case.

I did try Artem's patch that removes the offending code and that works 
fine.  No problems on any reboots or reading/writing the UBIFS.

Comments

Matt Mackall Jan. 27, 2010, 2:13 a.m.
On Tue, 2010-01-26 at 20:44 -0500, Jeff Angielski wrote:
> Artem Bityutskiy wrote:
> > On Mon, 2010-01-25 at 23:48 -0600, Matt Mackall wrote:
> >> Hmm. I'd just as soon drop it entirely. Here's a patch. Herbert, you
> >> want to send this through your crypto tree?
> >>
> >>
> >> random: drop weird m_time/a_time manipulation
> >>
> >> No other driver does anything remotely like this that I know of except
> >> for the tty drivers, and I can't see any reason for random/urandom to do
> >> it. In fact, it's a (trivial, harmless) timing information leak. And
> >> obviously, it generates power- and flash-cycle wasting I/O, especially
> >> if combined with something like hwrngd. Also, it breaks ubifs's
> >> expectations.
> >>
> >> Signed-off-by: Matt Mackall <mpm@selenic.com>
> >>
> >> diff -r 29db0c391ce8 drivers/char/random.c
> >> --- a/drivers/char/random.c	Sun Jan 17 11:01:16 2010 -0800
> >> +++ b/drivers/char/random.c	Mon Jan 25 23:32:00 2010 -0600
> >> @@ -1051,12 +1051,6 @@
> >>  				/* like a named pipe */
> >>  	}
> >>  
> >> -	/*
> >> -	 * If we gave the user some bytes, update the access time.
> >> -	 */
> >> -	if (count)
> >> -		file_accessed(file);
> >> -
> >>  	return (count ? count : retval);
> >>  }
> >>  
> >> @@ -1116,8 +1110,6 @@
> >>  	if (ret)
> >>  		return ret;
> >>  
> >> -	inode->i_mtime = current_fs_time(inode->i_sb);
> >> -	mark_inode_dirty(inode);
> >>  	return (ssize_t)count;
> >>  }
> > 
> > It may brake other FSes expectations, theoretically, as well.
> > 
> > Anyway, I'm perfectly fine if this is removed.
> > 
> > Jeff, could you please try Matt's patch and report back if you still
> > have issues or not. If no, you can use this as a temporary work-around
> > until a proper fix hits upstream or ubifs-2.6.git.
> 
> Matt's patch did not compile as written.  I tried to implement what I 
> think he was trying to do and created this patch (it seems to match the 
> guts of what inode_setattr() was looking for):
> 
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8258982..70f16c7 100644
> --- a/drivers/char/random.c
> +++ b/drivers/char/random.c
> @@ -1108,6 +1108,7 @@ static ssize_t random_write(struct file *file, 
> const char __user *buffer,
>   {
>   	size_t ret;
>   	struct inode *inode = file->f_path.dentry->d_inode;
> +	struct iattr attr;
> 
>   	ret = write_pool(&blocking_pool, buffer, count);
>   	if (ret)
> @@ -1116,8 +1117,12 @@ static ssize_t random_write(struct file *file, 
> const char __user *buffer,
>   	if (ret)
>   		return ret;
> 
> -	inode->i_mtime = current_fs_time(inode->i_sb);
> -	mark_inode_dirty(inode);
> +	attr.ia_mtime = current_fs_time(inode->i_sb);
> +	attr.ia_valid = ATTR_MTIME;
> +	ret = inode_setattr(inode, &attr);
> +	if (ret)
> +		return ret;
> +
>   	return (ssize_t)count;
>   }
> 
> However, this patch does not fix the problem.  I still see the same 
> errors.  Matt, is this what you were trying to do?

That doesn't look anything like my patch? And mine was test compiled.
Jeff Angielski Jan. 27, 2010, 3:07 a.m.
Matt Mackall wrote:
> On Tue, 2010-01-26 at 20:44 -0500, Jeff Angielski wrote:
>> Artem Bityutskiy wrote:
>>> On Mon, 2010-01-25 at 23:48 -0600, Matt Mackall wrote:
>>>> Hmm. I'd just as soon drop it entirely. Here's a patch. Herbert, you
>>>> want to send this through your crypto tree?
>>>>
>>>>
>>>> random: drop weird m_time/a_time manipulation
>>>>
>>>> No other driver does anything remotely like this that I know of except
>>>> for the tty drivers, and I can't see any reason for random/urandom to do
>>>> it. In fact, it's a (trivial, harmless) timing information leak. And
>>>> obviously, it generates power- and flash-cycle wasting I/O, especially
>>>> if combined with something like hwrngd. Also, it breaks ubifs's
>>>> expectations.
>>>>
>>>> Signed-off-by: Matt Mackall <mpm@selenic.com>
>>>>
>>>> diff -r 29db0c391ce8 drivers/char/random.c
>>>> --- a/drivers/char/random.c	Sun Jan 17 11:01:16 2010 -0800
>>>> +++ b/drivers/char/random.c	Mon Jan 25 23:32:00 2010 -0600
>>>> @@ -1051,12 +1051,6 @@
>>>>  				/* like a named pipe */
>>>>  	}
>>>>  
>>>> -	/*
>>>> -	 * If we gave the user some bytes, update the access time.
>>>> -	 */
>>>> -	if (count)
>>>> -		file_accessed(file);
>>>> -
>>>>  	return (count ? count : retval);
>>>>  }
>>>>  
>>>> @@ -1116,8 +1110,6 @@
>>>>  	if (ret)
>>>>  		return ret;
>>>>  
>>>> -	inode->i_mtime = current_fs_time(inode->i_sb);
>>>> -	mark_inode_dirty(inode);
>>>>  	return (ssize_t)count;
>>>>  }
>>> It may brake other FSes expectations, theoretically, as well.
>>>
>>> Anyway, I'm perfectly fine if this is removed.
>>>
>>> Jeff, could you please try Matt's patch and report back if you still
>>> have issues or not. If no, you can use this as a temporary work-around
>>> until a proper fix hits upstream or ubifs-2.6.git.
>> Matt's patch did not compile as written.  I tried to implement what I 
>> think he was trying to do and created this patch (it seems to match the 
>> guts of what inode_setattr() was looking for):
>>
>>
>> diff --git a/drivers/char/random.c b/drivers/char/random.c
>> index 8258982..70f16c7 100644
>> --- a/drivers/char/random.c
>> +++ b/drivers/char/random.c
>> @@ -1108,6 +1108,7 @@ static ssize_t random_write(struct file *file, 
>> const char __user *buffer,
>>   {
>>   	size_t ret;
>>   	struct inode *inode = file->f_path.dentry->d_inode;
>> +	struct iattr attr;
>>
>>   	ret = write_pool(&blocking_pool, buffer, count);
>>   	if (ret)
>> @@ -1116,8 +1117,12 @@ static ssize_t random_write(struct file *file, 
>> const char __user *buffer,
>>   	if (ret)
>>   		return ret;
>>
>> -	inode->i_mtime = current_fs_time(inode->i_sb);
>> -	mark_inode_dirty(inode);
>> +	attr.ia_mtime = current_fs_time(inode->i_sb);
>> +	attr.ia_valid = ATTR_MTIME;
>> +	ret = inode_setattr(inode, &attr);
>> +	if (ret)
>> +		return ret;
>> +
>>   	return (ssize_t)count;
>>   }
>>
>> However, this patch does not fix the problem.  I still see the same 
>> errors.  Matt, is this what you were trying to do?
> 
> That doesn't look anything like my patch? And mine was test compiled.

Ahh, you would be right.  I mixed up authors.  My bad.  ;)

Matt's patch that removes the offending code works fine.

Artem's patch that tries to fix the offending code (and does not compile 
as posted) does not work.
Artem Bityutskiy Jan. 27, 2010, 4:20 a.m.
On Tue, 2010-01-26 at 22:07 -0500, Jeff Angielski wrote:
> Matt Mackall wrote:
> > On Tue, 2010-01-26 at 20:44 -0500, Jeff Angielski wrote:
> >> Artem Bityutskiy wrote:
> >>> On Mon, 2010-01-25 at 23:48 -0600, Matt Mackall wrote:
> >>>> Hmm. I'd just as soon drop it entirely. Here's a patch. Herbert, you
> >>>> want to send this through your crypto tree?
> >>>>
> >>>>
> >>>> random: drop weird m_time/a_time manipulation
> >>>>
> >>>> No other driver does anything remotely like this that I know of except
> >>>> for the tty drivers, and I can't see any reason for random/urandom to do
> >>>> it. In fact, it's a (trivial, harmless) timing information leak. And
> >>>> obviously, it generates power- and flash-cycle wasting I/O, especially
> >>>> if combined with something like hwrngd. Also, it breaks ubifs's
> >>>> expectations.
> >>>>
> >>>> Signed-off-by: Matt Mackall <mpm@selenic.com>
> >>>>
> >>>> diff -r 29db0c391ce8 drivers/char/random.c
> >>>> --- a/drivers/char/random.c	Sun Jan 17 11:01:16 2010 -0800
> >>>> +++ b/drivers/char/random.c	Mon Jan 25 23:32:00 2010 -0600
> >>>> @@ -1051,12 +1051,6 @@
> >>>>  				/* like a named pipe */
> >>>>  	}
> >>>>  
> >>>> -	/*
> >>>> -	 * If we gave the user some bytes, update the access time.
> >>>> -	 */
> >>>> -	if (count)
> >>>> -		file_accessed(file);
> >>>> -
> >>>>  	return (count ? count : retval);
> >>>>  }
> >>>>  
> >>>> @@ -1116,8 +1110,6 @@
> >>>>  	if (ret)
> >>>>  		return ret;
> >>>>  
> >>>> -	inode->i_mtime = current_fs_time(inode->i_sb);
> >>>> -	mark_inode_dirty(inode);
> >>>>  	return (ssize_t)count;
> >>>>  }
> >>> It may brake other FSes expectations, theoretically, as well.
> >>>
> >>> Anyway, I'm perfectly fine if this is removed.
> >>>
> >>> Jeff, could you please try Matt's patch and report back if you still
> >>> have issues or not. If no, you can use this as a temporary work-around
> >>> until a proper fix hits upstream or ubifs-2.6.git.
> >> Matt's patch did not compile as written.  I tried to implement what I 
> >> think he was trying to do and created this patch (it seems to match the 
> >> guts of what inode_setattr() was looking for):
> >>
> >>
> >> diff --git a/drivers/char/random.c b/drivers/char/random.c
> >> index 8258982..70f16c7 100644
> >> --- a/drivers/char/random.c
> >> +++ b/drivers/char/random.c
> >> @@ -1108,6 +1108,7 @@ static ssize_t random_write(struct file *file, 
> >> const char __user *buffer,
> >>   {
> >>   	size_t ret;
> >>   	struct inode *inode = file->f_path.dentry->d_inode;
> >> +	struct iattr attr;
> >>
> >>   	ret = write_pool(&blocking_pool, buffer, count);
> >>   	if (ret)
> >> @@ -1116,8 +1117,12 @@ static ssize_t random_write(struct file *file, 
> >> const char __user *buffer,
> >>   	if (ret)
> >>   		return ret;
> >>
> >> -	inode->i_mtime = current_fs_time(inode->i_sb);
> >> -	mark_inode_dirty(inode);
> >> +	attr.ia_mtime = current_fs_time(inode->i_sb);
> >> +	attr.ia_valid = ATTR_MTIME;
> >> +	ret = inode_setattr(inode, &attr);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >>   	return (ssize_t)count;
> >>   }
> >>
> >> However, this patch does not fix the problem.  I still see the same 
> >> errors.  Matt, is this what you were trying to do?
> > 
> > That doesn't look anything like my patch? And mine was test compiled.
> 
> Ahh, you would be right.  I mixed up authors.  My bad.  ;)
> 
> Matt's patch that removes the offending code works fine.
> 
> Artem's patch that tries to fix the offending code (and does not compile 
> as posted) does not work.

Thanks for testing. So, who would bring Matt's patch upstream then, hmm?
Matt Mackall Jan. 27, 2010, 5:14 a.m.
On Wed, 2010-01-27 at 06:20 +0200, Artem Bityutskiy wrote:
> On Tue, 2010-01-26 at 22:07 -0500, Jeff Angielski wrote:
> > Matt Mackall wrote:
> > > On Tue, 2010-01-26 at 20:44 -0500, Jeff Angielski wrote:
> > >> Artem Bityutskiy wrote:
> > >>> On Mon, 2010-01-25 at 23:48 -0600, Matt Mackall wrote:
> > >>>> Hmm. I'd just as soon drop it entirely. Here's a patch. Herbert, you
> > >>>> want to send this through your crypto tree?
> > >>>>
> > >>>>
> > >>>> random: drop weird m_time/a_time manipulation
> > >>>>
> > >>>> No other driver does anything remotely like this that I know of except
> > >>>> for the tty drivers, and I can't see any reason for random/urandom to do
> > >>>> it. In fact, it's a (trivial, harmless) timing information leak. And
> > >>>> obviously, it generates power- and flash-cycle wasting I/O, especially
> > >>>> if combined with something like hwrngd. Also, it breaks ubifs's
> > >>>> expectations.
> > >>>>
> > >>>> Signed-off-by: Matt Mackall <mpm@selenic.com>
> > >>>>
> > >>>> diff -r 29db0c391ce8 drivers/char/random.c
> > >>>> --- a/drivers/char/random.c	Sun Jan 17 11:01:16 2010 -0800
> > >>>> +++ b/drivers/char/random.c	Mon Jan 25 23:32:00 2010 -0600
> > >>>> @@ -1051,12 +1051,6 @@
> > >>>>  				/* like a named pipe */
> > >>>>  	}
> > >>>>  
> > >>>> -	/*
> > >>>> -	 * If we gave the user some bytes, update the access time.
> > >>>> -	 */
> > >>>> -	if (count)
> > >>>> -		file_accessed(file);
> > >>>> -
> > >>>>  	return (count ? count : retval);
> > >>>>  }
> > >>>>  
> > >>>> @@ -1116,8 +1110,6 @@
> > >>>>  	if (ret)
> > >>>>  		return ret;
> > >>>>  
> > >>>> -	inode->i_mtime = current_fs_time(inode->i_sb);
> > >>>> -	mark_inode_dirty(inode);
> > >>>>  	return (ssize_t)count;
> > >>>>  }
> > >>> It may brake other FSes expectations, theoretically, as well.
> > >>>
> > >>> Anyway, I'm perfectly fine if this is removed.
> > >>>
> > >>> Jeff, could you please try Matt's patch and report back if you still
> > >>> have issues or not. If no, you can use this as a temporary work-around
> > >>> until a proper fix hits upstream or ubifs-2.6.git.
> > >> Matt's patch did not compile as written.  I tried to implement what I 
> > >> think he was trying to do and created this patch (it seems to match the 
> > >> guts of what inode_setattr() was looking for):
> > >>
> > >>
> > >> diff --git a/drivers/char/random.c b/drivers/char/random.c
> > >> index 8258982..70f16c7 100644
> > >> --- a/drivers/char/random.c
> > >> +++ b/drivers/char/random.c
> > >> @@ -1108,6 +1108,7 @@ static ssize_t random_write(struct file *file, 
> > >> const char __user *buffer,
> > >>   {
> > >>   	size_t ret;
> > >>   	struct inode *inode = file->f_path.dentry->d_inode;
> > >> +	struct iattr attr;
> > >>
> > >>   	ret = write_pool(&blocking_pool, buffer, count);
> > >>   	if (ret)
> > >> @@ -1116,8 +1117,12 @@ static ssize_t random_write(struct file *file, 
> > >> const char __user *buffer,
> > >>   	if (ret)
> > >>   		return ret;
> > >>
> > >> -	inode->i_mtime = current_fs_time(inode->i_sb);
> > >> -	mark_inode_dirty(inode);
> > >> +	attr.ia_mtime = current_fs_time(inode->i_sb);
> > >> +	attr.ia_valid = ATTR_MTIME;
> > >> +	ret = inode_setattr(inode, &attr);
> > >> +	if (ret)
> > >> +		return ret;
> > >> +
> > >>   	return (ssize_t)count;
> > >>   }
> > >>
> > >> However, this patch does not fix the problem.  I still see the same 
> > >> errors.  Matt, is this what you were trying to do?
> > > 
> > > That doesn't look anything like my patch? And mine was test compiled.
> > 
> > Ahh, you would be right.  I mixed up authors.  My bad.  ;)
> > 
> > Matt's patch that removes the offending code works fine.
> > 
> > Artem's patch that tries to fix the offending code (and does not compile 
> > as posted) does not work.
> 
> Thanks for testing. So, who would bring Matt's patch upstream then, hmm?
> 

I think Herbert's tree is the best path, but if he doesn't chime in,
I'll send it through Andrew.
Herbert Xu Jan. 29, 2010, 11:27 a.m.
On Wed, Jan 27, 2010 at 06:20:10AM +0200, Artem Bityutskiy wrote:
>
> Thanks for testing. So, who would bring Matt's patch upstream then, hmm?

I've got it in crypto-2.6.  Thanks!
Artem Bityutskiy Jan. 29, 2010, 11:32 a.m.
On Fri, 2010-01-29 at 22:27 +1100, Herbert Xu wrote:
> On Wed, Jan 27, 2010 at 06:20:10AM +0200, Artem Bityutskiy wrote:
> >
> > Thanks for testing. So, who would bring Matt's patch upstream then, hmm?
> 
> I've got it in crypto-2.6.  Thanks!

I'd appreciate an URL.

I've just cloned:

git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6

and do not see it there.
Herbert Xu Jan. 29, 2010, 11:35 a.m.
On Fri, Jan 29, 2010 at 01:32:09PM +0200, Artem Bityutskiy wrote:
> 
> I've just cloned:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6
> 
> and do not see it there.

Should show up soon once my push makes it through.

Cheers,
Artem Bityutskiy Feb. 2, 2010, 10:42 a.m.
On Tue, 2010-01-26 at 22:07 -0500, Jeff Angielski wrote:
... snip ...
> Matt's patch that removes the offending code works fine.
... snip ...

Jeff, I've pushed the patches from Herbert's tree to the ubifs back-port
trees (ubifs-v2.6.28 - ubifs-v2.6.32).

Thanks everyone.

Patch hide | download patch | download mbox

diff --git a/drivers/char/random.c b/drivers/char/random.c
index 8258982..70f16c7 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1108,6 +1108,7 @@  static ssize_t random_write(struct file *file, 
const char __user *buffer,
  {
  	size_t ret;
  	struct inode *inode = file->f_path.dentry->d_inode;
+	struct iattr attr;

  	ret = write_pool(&blocking_pool, buffer, count);