linux-kernel - Re: [LTP/VFS] fcntl SETLEASE fails on ramfs/tmpfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 29 Apr 2008 13:54:54 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	"Bryan Wu" <cooloney@...nel.org>
Cc:	linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
	willy@...ian.org, viro@...iv.linux.org.uk,
	uclinux-dist-devel@...ckfin.uclinux.org,
	"J. Bruce Fields" <bfields@...ldses.org>
Subject: Re: [LTP/VFS] fcntl SETLEASE fails on ramfs/tmpfs

On Tue, 29 Apr 2008 11:42:48 +0800
"Bryan Wu" <cooloney@...nel.org> wrote:

> Hi folk,
> 
> This days I am digging into this LTP bug reported on our Blackfin test
> machine, but I think it is general for other system.
> https://blackfin.uclinux.org/gf/project/uclinux-dist/tracker/?action=TrackerItemEdit&tracker_id=141&tracker_item_id=3743
> 
> And I also found Kumar Gala reported this similar bug before.
> http://lkml.org/lkml/2007/11/14/388
> 
> 1, when opening and creating a new on ramfs/tmpfs, the dentry->d_count
> will be added one as below:
> --
> ramfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
> {
> |_______struct inode * inode = ramfs_get_inode(dir->i_sb, mode, dev);
> |_______int error = -ENOSPC;
> 
> |_______if (inode) {
> |_______|_______if (dir->i_mode & S_ISGID) {
> |_______|_______|_______inode->i_gid = dir->i_gid;
> |_______|_______|_______if (S_ISDIR(mode))
> |_______|_______|_______|_______inode->i_mode |= S_ISGID;
> |_______|_______}
> |_______|_______d_instantiate(dentry, inode);
> |_______|_______dget(dentry);|__/* Extra count - pin the dentry in core */
> |_______|_______error = 0;
> |_______|_______dir->i_mtime = dir->i_ctime = CURRENT_TIME;
> |_______}
> |_______return error;
> }
> --
> The dget(dentry) call introduces an extra count, why?
> it is the same in tmpfs.

Because those dentries have no backing store.  Their sole existance is in
the dentry cache which is normally reclaimable.  But we can't reclaim these
dentries because there is nowhere from where they can be reestablished.

> 2, when calling  fcntl(fd, F_SETLEASE,F_WRLCK), it will return -EAGAIN
> --
> |_______if ((arg == F_WRLCK)
> |_______    && ((atomic_read(&dentry->d_count) > 1)
> |_______|_______|| (atomic_read(&inode->i_count) > 1)))
> |_______|_______goto out;
> --

Sucky heuristic.

> because the dentry->d_count will be 2 not 1. I tested ext2 on Blackfin, it is 1.
> 
> 3, so I guess maybe the dget(dentry) of ramfs_mknod is useless. But
> after remove this dget(),
> the ramfs can not be mounted as rootfs at all.

Interesting.  Presumably it got reclaimed synchronously somehow.

> Is the bug in generic_setlease() or in the ramfs/tmpfs inode create function?
> 
> Of course, simply remove the test '((atomic_read(&dentry->d_count) >
> 1)' can workaround this issue.

I guess we should make the generic_setlease() heuristic smarter.

Of course the _reason_ for that heuristic is uncommented and lost in time. 
And one wonders what locking prevents it from being totally racy, and if
"none", what happens when the race hits.  Sigh.

I suppose a stupid fix would be to set (and later clear) a new flag in
dentry.d_flags which means

  this dentry is pinned by a ram-backed device, so d_count==2 means
  "unused""

But it would be better to work out exactly what generic_setlease() is
trying to do there, and do it in a better way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/