Hello David,
I was reading your statx(2) man page, and noticed this text:
Do not simply set mask to UINT_MAX as one or more bits may, in the
future, be used to specify an extension to the buffer.
(Here' 'mask' is the fourth argument to statx())
What is going on here? Why is there not a check in the code to
give EINVAL if any flag other than those in STATX_ALL (0x00000fffU)
is specified? (There is a check that gives EINVAL flags in
STATX__RESERVED (0x80000000U), but STATX_ALL != ~STATX__RESERVED.
Similarly, there appears to be no check for invalid flags in the
'flags' argument of statx(). Why is there also not such a check
there?
The failure to do these sorts of checks has been the source of grief
in the past with other system calls.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
[Adding a few people to CC, and correcting myself on one piece]
On 04/21/2017 02:14 PM, Michael Kerrisk (man-pages) wrote:
> Hello David,
>
> I was reading your statx(2) man page, and noticed this text:
>
> Do not simply set mask to UINT_MAX as one or more bits may, in the
> future, be used to specify an extension to the buffer.
>
> (Here' 'mask' is the fourth argument to statx())
>
> What is going on here? Why is there not a check in the code to
> give EINVAL if any flag other than those in STATX_ALL (0x00000fffU)
> is specified? (There is a check that gives EINVAL flags in
> STATX__RESERVED (0x80000000U), but STATX_ALL != ~STATX__RESERVED.
>
> Similarly, there appears to be no check for invalid flags in the
> 'flags' argument of statx(). Why is there also not such a check
> there?
>
> The failure to do these sorts of checks has been the source of grief
> in the past with other system calls.
So, it looks like the checks are there for 'flags' (I missed that
they were one level deeper in the call sequence), but my question
re 'mask' still stands.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
Michael Kerrisk (man-pages) <[email protected]> wrote:
> I was reading your statx(2) man page, and noticed this text:
>
> Do not simply set mask to UINT_MAX as one or more bits may, in the
> future, be used to specify an extension to the buffer.
>
> (Here' 'mask' is the fourth argument to statx())
>
> What is going on here? Why is there not a check in the code to
> give EINVAL if any flag other than those in STATX_ALL (0x00000fffU)
> is specified? (There is a check that gives EINVAL flags in
> STATX__RESERVED (0x80000000U), but STATX_ALL != ~STATX__RESERVED.
Yeah, I need to update that. I sent you the manpage to have a look at before
the patch that added the reservation got merged - possibly before I even wrote
that patch.
> Similarly, there appears to be no check for invalid flags in the
> 'flags' argument of statx(). Why is there also not such a check
> there?
Like this?
if (mask & STATX__RESERVED)
return -EINVAL;
David
David Howells <[email protected]> wrote:
> > Similarly, there appears to be no check for invalid flags in the
> > 'flags' argument of statx(). Why is there also not such a check
> > there?
>
> Like this?
>
> if (mask & STATX__RESERVED)
> return -EINVAL;
Sorry, I misread. You referred to flags, not mask. There's this in
sys_statx:
if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
return -EINVAL;
this in vfs_statx:
if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_NO_AUTOMOUNT |
AT_EMPTY_PATH | KSTAT_QUERY_FLAGS)) != 0)
return -EINVAL;
and this in vfs_statx_fd:
if (query_flags & ~KSTAT_QUERY_FLAGS)
return -EINVAL;
I don't necessarily agree with that last one, but other people think it should
be there.
David
Michael Kerrisk (man-pages) <[email protected]> wrote:
> I was reading your statx(2) man page, and noticed this text:
>
> Do not simply set mask to UINT_MAX as one or more bits may, in the
> future, be used to specify an extension to the buffer.
>
> (Here' 'mask' is the fourth argument to statx())
>
> What is going on here? Why is there not a check in the code to
> give EINVAL if any flag other than those in STATX_ALL (0x00000fffU)
> is specified? (There is a check that gives EINVAL flags in
> STATX__RESERVED (0x80000000U), but STATX_ALL != ~STATX__RESERVED.
Because:
(1) There's no way to find out what mask bits are supported by the kernel,
short of trying them, and all the kernel tells you is that you were wrong
somehow. It's kind of like playing the mastermind game.
(2) What an application sees as STATX_ALL is provided by the header files it
was built against, not the kernel it is being run against. The running
kernel may have a different idea of what STATX_ALL should be. Further,
the running kernel may provide more, or _less_, than an app was lead to
expect.
(3) There's no problem with asking for extra bits, even if the running kernel
does not support them, because the kernel tells you in its response what
fields it actually gave you. If you asked for a field that didn't exist,
the flag will be clear in stx_mask upon return.
One thing the kernel guarantees is at least some support for all fields
governed by STATX_BASIC_STATS, ie. the fields that correspond to those in
the normal stat struct - and that any such field will have a dummy value
emplaced if the field isn't supported (exactly as with stat() now).
This is documented thusly:
.PP
It should be noted that the kernel may return fields that weren't requested and
may fail to return fields that were requested, depending on what the backing
filesystem supports. In either case,
.I stx_mask
will not be equal
.IR mask .
.PP
If a filesystem does not support a field or if it has an unrepresentable value
(for instance, a file with an exotic type), then the mask bit corresponding to
that field will be cleared in
.I stx_mask
even if the user asked for it and a dummy value will be filled in for
compatibility purposes if one is available (e.g. a dummy uid and gid may be
specified to mount under some circumstances).
.PP
A filesystem may also fill in fields that the caller didn't ask for if it has
values for them available at no extra cost. If this happens, the corresponding
bits will be set in
.IR stx_mask .
.PP
David
On 04/21/2017 03:01 PM, David Howells wrote:
> Michael Kerrisk (man-pages) <[email protected]> wrote:
> (3) There's no problem with asking for extra bits, even if the running kernel
> does not support them, because the kernel tells you in its response what
> fields it actually gave you.
It's this piece that I overlooked. Makes sense, of course.
Sorry for the noise!
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
On Apr 21, 2017, at 7:13 AM, Michael Kerrisk (man-pages) <[email protected]> wrote:
>
> On 04/21/2017 03:01 PM, David Howells wrote:
>> Michael Kerrisk (man-pages) <[email protected]> wrote:
>
>> (3) There's no problem with asking for extra bits, even if the running
>> kerneldoes not support them, because the kernel tells you in its
>> response what fields it actually gave you.
>
> It's this piece that I overlooked. Makes sense, of course.
> Sorry for the noise!
I agree with David that we don't want to return an error if the application
asks for more bits than the kernel supports, otherwise the interface would
be useless.
Maybe this implies that this needs to be explained more clearly in the
statx man page?
Cheers, Andreas
Hello Andreas,
On 04/21/2017 08:16 PM, Andreas Dilger wrote:
> On Apr 21, 2017, at 7:13 AM, Michael Kerrisk (man-pages) <[email protected]> wrote:
>>
>> On 04/21/2017 03:01 PM, David Howells wrote:
>>> Michael Kerrisk (man-pages) <[email protected]> wrote:
>>
>>> (3) There's no problem with asking for extra bits, even if the running
>>> kerneldoes not support them, because the kernel tells you in its
>>> response what fields it actually gave you.
>>
>> It's this piece that I overlooked. Makes sense, of course.
>> Sorry for the noise!
>
> I agree with David that we don't want to return an error if the application
> asks for more bits than the kernel supports, otherwise the interface would
> be useless.
Yes, it's clear to me now.
> Maybe this implies that this needs to be explained more clearly in the
> statx man page?
Precisely; my thought also.
Cheers,
Michael
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/