On Tue, Aug 15, 2017 at 5:06 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > On Tue, Aug 15, 2017 at 4:52 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: >> On Tue, Aug 15, 2017 at 3:35 PM, Miklos Szeredi <miklos@xxxxxxxxxx> wrote: >>> On Tue, Aug 08, 2017 at 07:01:30AM +0200, Amir Goldstein wrote: >>>> On Mon, Aug 7, 2017 at 9:57 AM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote: >>> >>> [snip] >>> >>>> > 2. Chattr will modify lower file's attributes directly. >>>> > Reproduce: >>>> > # mkdir lower upper worker merger >>>> > # touch lower/aa >>>> > # lsattr -p lower/aa >>>> > 0 --------------e---- lower/aa >>>> > # mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=worker overlayfs merger >>>> > # chattr -p 123 merger/aa #set project id >>>> > # lsattr -p lower/aa >>>> > 123 --------------e---- lower/aa >>>> > >>>> > If we try to set "immutable" or any other attributes, the result are consistent. >>>> > Because chattr open file in RDONLY mode, so it will not trigger copyup, and then, >>>> > FS_IOC_SETFLAGS ioctl will get the lower inode and modify it. >>>> >>>> Ouch! I guess it's a "known to some" issue. >>>> Fixing this would be a pain (intercept ioctl and whitelisting readonly >>>> fs specific ioctls). >>> >>> Fixing ioctl properly would be a pain. But we can hack around the issue, and >>> just deny it for now. >>> >>> See patch below >> >> I like this, but it will require good test coverage of fs specific ioctls. >> The list of filesystems that call mnt_want_write_file() for ioctl is not short. > > If it's called from within the filesystem, then the new behavior is > certainly the correct one. It certainly is. It doesn't mean that fixing incorrect behavior won't lead to unacceptable regressions, which may require explicit d_real() call from filesystem to be fixed. Side note: IMO may_write_real() should return -EPERM instead of -EINVAL, same behavior as IS_IMMUTABLE, i.e. there is nothing invalid about the parameters of the SETFLAGS ioctl as far as the user is concerned. You could also go with -EROFS, which makes more sence, but that may be a bit more surprising to user. Amir. -- To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
