Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1628194 > unrolled thread

Unchecked flags in statx(2) [Should be fixed before 4.11-final?]

Back to article view | Back to linux.kernel

Contents

  Unchecked flags in statx(2) [Should be fixed before 4.11-final?] "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2017-04-21 14:20 +0200
    Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2017-04-21 14:30 +0200
    Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] David Howells <dhowells@redhat.com> - 2017-04-21 14:50 +0200
    Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] David Howells <dhowells@redhat.com> - 2017-04-21 14:50 +0200
    Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] David Howells <dhowells@redhat.com> - 2017-04-21 15:10 +0200
      Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2017-04-21 15:20 +0200
        Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] Andreas Dilger <adilger@dilger.ca> - 2017-04-21 20:50 +0200
          Re: Unchecked flags in statx(2) [Should be fixed before 4.11-final?] "Michael Kerrisk (man-pages)" <mtk.manpages@gmail.com> - 2017-04-21 21:20 +0200

#1628194 — Unchecked flags in statx(2) [Should be fixed before 4.11-final?]

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/

[toc] | [next] | [standalone]

#1628202

[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/

[toc] | [prev] | [next] | [standalone]

#1628210

David Howells <dhowells@redhat.com> 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

[toc] | [prev] | [next] | [standalone]

#1628213

Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1628235

Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1628246

On 04/21/2017 03:01 PM, David Howells wrote:
> Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> 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/

[toc] | [prev] | [next] | [standalone]

#1628488

[Multipart message — attachments visible in raw view] — view raw

On Apr 21, 2017, at 7:13 AM, Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> wrote:
> 
> On 04/21/2017 03:01 PM, David Howells wrote:
>> Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> 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

[toc] | [prev] | [next] | [standalone]

#1628525

Hello Andreas,

On 04/21/2017 08:16 PM, Andreas Dilger wrote:
> On Apr 21, 2017, at 7:13 AM, Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> wrote:
>>
>> On 04/21/2017 03:01 PM, David Howells wrote:
>>> Michael Kerrisk (man-pages) <mtk.manpages@gmail.com> 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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web