Re: UBIFS assert failed in ubifs_dirty_inode

Matt Mackall
2010-01-26T05:48:59Z

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>

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;
}

--
http://selenic.com : development and support for Mercurial and Linux