UBIFS assert failed in ubifs_dirty_inode

On Tue, 2010-01-26 at 06:40 +0200, Artem Bityutskiy wrote:
> On Thu, 2010-01-21 at 22:03 -0500, Jeff Angielski wrote:
> > I am trying use an UBIFS root filesystem on my PowerPC MPC8544 but I am 
> > seeing some intermitent problems with "UBIFS assert failed in 
> > ubifs_dirty_inode" errors.
> > 
> > On the first boot after I program the NAND with a fresh UBI image, 
> > everything seems to work ok.
> > 
> > After that, on subsequent powercycles or reboots, I sometimes see a boot 
> > with the following error:
> > 
> > [    5.984232] UBIFS assert failed in ubifs_dirty_inode at 377 (pid 1011)
> 
> Interesting.
> 
> The stack trace for this assertion is:
> 
> [   42.724193] [df121e60] [c00070f8] show_stack+0x3c/0x17c (unreliable)
> [   42.730566] [df121ea0] [c017e754] ubifs_dirty_inode+0xe0/0xe4
> [   42.736325] [df121eb0] [c00c6fbc] __mark_inode_dirty+0x3c/0x16c
> [   42.742260] [df121ec0] [c01f9034] random_write+0x8c/0xa4
> [   42.747584] [df121ef0] [c00a4d2c] vfs_write+0xb4/0x184
> [   42.752730] [df121f10] [c00a53e8] sys_write+0x4c/0x90
> [   42.757795] [df121f40] [c000fd48] ret_from_syscall+0x0/0x3c
> 
> Which leads us to
> 
> ~/git/linux-2.6/drivers/char/random.c, where we can find this code:
> 
>         inode->i_mtime = current_fs_time(inode->i_sb);
>         mark_inode_dirty(inode);
>         return (ssize_t)count;
> 
> which is the reason for the assertion and for the further budgeting
> screw-up.
> 
> The thing is that UBIFS has to allocate budget every-time a clean inode
> is made dirty. But the 'random' driver bypasses UBIFS budget allocation,
> and instead, changes mtime directly and marks the inode as dirty
> directly.
> 
> The driver should instead call the ->setattr() method of the inode,
> which should do the right thing. IOW, something like this is needed:
> 
> diff --git a/drivers/char/random.c b/drivers/char/random.c
> index 8258982..f911781 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;
> 
>         ret = write_pool(&blocking_pool, buffer, count);
>         if (ret)
> @@ -1116,8 +1117,11 @@ 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);
> +       iattr->i_mtime = current_fs_time(inode->i_sb);
> +       iattr->ia_valid = ATTR_ATIME;
> +       ret = inode_setattr(inode, &attr);
> +       if (ret)
> +               return ret;
>         return (ssize_t)count;
>  }
> 
> Note - I did not even compile-test this. Would you try that please :-)

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>

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.

Thanks!

On Tue, 2010-01-26 at 12:03 +0200, 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>

Just in case anyone wonders where this came from, here is the beginning
of the thread:

http://lists.infradead.org/pipermail/linux-mtd/2010-January/028727.html

On Mon, Jan 25, 2010 at 11:48:48PM -0600, Matt Mackall wrote:
>
> 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>

Thanks Matt, I'll add this to crypto-2.6 and then stable.