On Thu, Aug 17, 2017 at 4:55 AM, zhangyi (F) <yi.zhang@xxxxxxxxxx> wrote:
> On 2017/8/16 19:10, Amir Goldstein Wrote:
...
>>>> I guess that would make sense.
>>>> I only wonder what was the reason for chattr to open RDONLY in
>>>> the first place (cc Ted)??
>>>
>>> What about copy up of flags? Should we?
>>
>> IMO we should start with copying KSTAT_ATTR_FS_IOC_FLAGS,
>> which can already be available in kstat.
>> Other "standard" FS_IOC_FLAGS can be copied up when statx
>> gets support for those flags.
>>
>> FYI, chattr [+/-]<flags> does:
>> - fgetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_GETFLAGS}
>> - change flags
>> - fsetflags(&flags): {open OPEN_FLAGS; ioctl EXT2_IOC_SETFLAGS}
>>
>> OPEN_FLAGS is defined independently in lib/e2p/f{get,set}flags.c as
>> (O_RDONLY|O_NONBLOCK|O_LARGEFILE), so technically all it
>> takes is to change the defined in lib/e2p/fsetflags.c to O_RDWR
>> and then chattr [+/-]<flags> can be used for preemptive "copy up flags"
>> by userspace (*) to work around issues which require flags copy up.
>>
>
> Hi Amir:
> I notice that RDONLY is required if file is immutable or append-only,
> chattr will fail with EPERM return value if we use O_RDWR open flag to open
> a immutable or append-only file. So I think we cannot simply change
> to O_RDWR in lib/e2p/fsetflags.c, we should take care about this special,
> please see check in may_open() and __inode_permission().
>
Zhangyi,
You are right about the fact that we cannot "just change" OPEN_FLAGS
to O_RDWR, because it would return EPERM for non-overlayfs case and
also in some cases for overlayfs upper file.
However, the workaround for userspace to open O_RDWR to trigger
copy up before chattr should still work because may_open() has no
knowledge of the real inode flags (open may fail but still copy up). See
commit b0990fbbbd14 ("ovl: check IS_APPEND() on real upper inode")
Note that this commit does NOT prevent opening a lower file
O_RDWR even if that file is IS_APPEND or IS_IMMUTABLE
as mentioned in the commit message and that the reason this
commit only speaks about IS_APPEND() is that IS_MMUTABLE
is also checked later with inode_permission(MAY_WRITE).
(*) Like my answer before, this is all in theory, so I may be missing
something... again.
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
