On Tue, Aug 15, 2017 at 5:28 PM, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > 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. I don't get it. The only possible regression is denying modification on lower layer where previously was allowed. But anybody relying on that would be pretty crazy. > > 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. Yeah, EPERM sounds better. Thanks, Miklos -- 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
