On Tue, Aug 15, 2017 at 5:56 PM, Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > On Tue, Aug 15, 2017 at 05:33:01PM +0200, Miklos Szeredi wrote: >> 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. > > Hi Miklos, > > IIUC, so now "chattr -p <id>" will fail on overlayfs (assume file has not > been copied up yet). > Yap. > IOW, on overlayfs, will it be responsibility of user space to make > sure file has been copied up, for chattr operation to succeed? Does that > mean we need to modify chattr to open file for WRITE instead of READ. > I guess that would make sense. I only wonder what was the reason for chattr to open RDONLY in the first place (cc Ted)?? 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
