Messages in this thread | | | Subject | Re: [PATCH 09/10] VFS: Remove read-only checks from dentry_permission | From | Miklos Szeredi <> | Date | Wed, 08 Sep 2010 10:58:17 +0200 |
| |
On Wed, 8 Sep 2010, Neil Brown wrote: > On Mon, 06 Sep 2010 21:10:10 +0200 > Miklos Szeredi <miklos@szeredi.hu> wrote: > > > On Mon, 06 Sep 2010, NeilBrown wrote: > > > It is not sufficient to depend on the the "filesystem is readonly" > > > tests in dentry_permission as it does not check if the vfsmnt is > > > readonly. > > > All call sites already call mnt_want_write or __mnt_is_readonly which > > > includes the test on MS_RDONLY. > > > > Last time I checked I found some holes (in nfsd IIRC). That was a > > long time ago and things may have changed. > > nfsd looks OK to me. I didn't do an exhaustive audit but couldn't find > things that would not still work correctly.
Looks to me as if nfsd_setattr() and some of its callers fail to acquire a write ref.
> > > > That check could be replaced with a > > > > if (IS_RDONLY(inode) && > > (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode))) > > BUG(); > > That wouldn't quite work currently. > sys_faccessat checks __mnt_is_readonly *after* the call to dentry_permission,
Ugh.
> so the above would cause it to BUG. Possibly the __mnt_is_readonly could be > checked before dentry_permission. > > However nfsd_permission is a bit more awkward to fix as sometimes it > deliberately wants to ignore read-only-filesystem issues ... but it might > still be possible to work around.. > > > > > which would catch these cases but only if the superblock was marked > > r/o. Otherwise it's basically impossible to make sure the callers of > > the VFS play by the rules. That was one reason I advocated a > > path_... interface for the VFS instead of the current dentry based > > one, but Al didn't like it. > > > > I guess there comes a point were we just have to document the rules and if > someone doesn't play by them - that is a bug...
Yeah and that's fine as long as the bug shows up relatively easily and not result in silent fs corruption once in blue moon instead.
If that MS_RDONLY check is removed the above criterion will not be met.
Thanks, Miklos
| |