Re: [PATCH v2] uapi/fcntl: define RENAME_* and AT_RENAME_* macros

[Randy Dunlap <rdunlap@infradead.org>]

Hi,

On 9/1/25 11:58 PM, Amir Goldstein wrote:
> On Tue, Sep 2, 2025 at 1:14 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>>
>> Define the RENAME_* and AT_RENAME_* macros exactly the same as in
>> recent glibc <stdio.h> so that duplicate definition build errors in
>> both samples/watch_queue/watch_test.c and samples/vfs/test-statx.c
>> no longer happen. When they defined in exactly the same way in
>> multiple places, the build errors are prevented.
>>
>> Defining only the AT_RENAME_* macros is not sufficient since they
>> depend on the RENAME_* macros, which may not be defined when the
>> AT_RENAME_* macros are used.
>>
>> Build errors being fixed:
>>
>> for samples/vfs/test-statx.c:
>>
>> In file included from ../samples/vfs/test-statx.c:23:
>> usr/include/linux/fcntl.h:159:9: warning: ‘AT_RENAME_NOREPLACE’ redefined
>>   159 | #define AT_RENAME_NOREPLACE     0x0001
>> In file included from ../samples/vfs/test-statx.c:13:
>> /usr/include/stdio.h:171:10: note: this is the location of the previous definition
>>   171 | # define AT_RENAME_NOREPLACE RENAME_NOREPLACE
>> usr/include/linux/fcntl.h:160:9: warning: ‘AT_RENAME_EXCHANGE’ redefined
>>   160 | #define AT_RENAME_EXCHANGE      0x0002
>> /usr/include/stdio.h:173:10: note: this is the location of the previous definition
>>   173 | # define AT_RENAME_EXCHANGE RENAME_EXCHANGE
>> usr/include/linux/fcntl.h:161:9: warning: ‘AT_RENAME_WHITEOUT’ redefined
>>   161 | #define AT_RENAME_WHITEOUT      0x0004
>> /usr/include/stdio.h:175:10: note: this is the location of the previous definition
>>   175 | # define AT_RENAME_WHITEOUT RENAME_WHITEOUT
>>
>> for samples/watch_queue/watch_test.c:
>>
>> In file included from usr/include/linux/watch_queue.h:6,
>>                  from ../samples/watch_queue/watch_test.c:19:
>> usr/include/linux/fcntl.h:159:9: warning: ‘AT_RENAME_NOREPLACE’ redefined
>>   159 | #define AT_RENAME_NOREPLACE     0x0001
>> In file included from ../samples/watch_queue/watch_test.c:11:
>> /usr/include/stdio.h:171:10: note: this is the location of the previous definition
>>   171 | # define AT_RENAME_NOREPLACE RENAME_NOREPLACE
>> usr/include/linux/fcntl.h:160:9: warning: ‘AT_RENAME_EXCHANGE’ redefined
>>   160 | #define AT_RENAME_EXCHANGE      0x0002
>> /usr/include/stdio.h:173:10: note: this is the location of the previous definition
>>   173 | # define AT_RENAME_EXCHANGE RENAME_EXCHANGE
>> usr/include/linux/fcntl.h:161:9: warning: ‘AT_RENAME_WHITEOUT’ redefined
>>   161 | #define AT_RENAME_WHITEOUT      0x0004
>> /usr/include/stdio.h:175:10: note: this is the location of the previous definition
>>   175 | # define AT_RENAME_WHITEOUT RENAME_WHITEOUT
>>
>> Fixes: b4fef22c2fb9 ("uapi: explain how per-syscall AT_* flags should be allocated")
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>> ---
>> Cc: Amir Goldstein <amir73il@gmail.com>
>> Cc: Jeff Layton <jlayton@kernel.org>
>> Cc: Chuck Lever <chuck.lever@oracle.com>
>> Cc: Alexander Aring <alex.aring@gmail.com>
>> Cc: Josef Bacik <josef@toxicpanda.com>
>> Cc: Aleksa Sarai <cyphar@cyphar.com>
>> Cc: Jan Kara <jack@suse.cz>
>> Cc: Christian Brauner <brauner@kernel.org>
>> Cc: Matthew Wilcox <willy@infradead.org>
>> Cc: David Howells <dhowells@redhat.com>
>> CC: linux-api@vger.kernel.org
>> To: linux-fsdevel@vger.kernel.org
>>
>>  include/uapi/linux/fcntl.h |    9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> --- linux-next-20250819.orig/include/uapi/linux/fcntl.h
>> +++ linux-next-20250819/include/uapi/linux/fcntl.h
>> @@ -156,9 +156,12 @@
>>   */
>>
>>  /* Flags for renameat2(2) (must match legacy RENAME_* flags). */
>> -#define AT_RENAME_NOREPLACE    0x0001
>> -#define AT_RENAME_EXCHANGE     0x0002
>> -#define AT_RENAME_WHITEOUT     0x0004
>> +# define RENAME_NOREPLACE (1 << 0)
>> +# define AT_RENAME_NOREPLACE RENAME_NOREPLACE
>> +# define RENAME_EXCHANGE (1 << 1)
>> +# define AT_RENAME_EXCHANGE RENAME_EXCHANGE
>> +# define RENAME_WHITEOUT (1 << 2)
>> +# define AT_RENAME_WHITEOUT RENAME_WHITEOUT
>>
> 
> This solution, apart from being terribly wrong (adjust the source to match
> to value of its downstream copy), does not address the issue that Mathew
> pointed out on v1 discussion [1]:

I didn't forget or ignore this.
If the macros have the same values (well, not just values but also the
same text), then I don't see why it matters whether they are in some older
version of glibc.

> $ grep -r AT_RENAME_NOREPLACE /usr/include
> /usr/include/linux/fcntl.h:#define AT_RENAME_NOREPLACE  0x0001
> 
> It's not in stdio.h at all.  This is with libc6 2.41-10
> 
> [1] https://lore.kernel.org/linux-fsdevel/aKxfGix_o4glz8-Z@casper.infradead.org/
> 
> I don't know how to resolve the mess that glibc has created.

Yeah, I guess I don't either.

> Perhaps like this:
> 
> diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
> index f291ab4f94ebc..dde14fa3c2007 100644
> --- a/include/uapi/linux/fcntl.h
> +++ b/include/uapi/linux/fcntl.h
> @@ -155,10 +155,16 @@
>   * as possible, so we can use them for generic bits in the future if necessary.
>   */
> 
> -/* Flags for renameat2(2) (must match legacy RENAME_* flags). */
> -#define AT_RENAME_NOREPLACE    0x0001
> -#define AT_RENAME_EXCHANGE     0x0002
> -#define AT_RENAME_WHITEOUT     0x0004
> +/*
> + * The legacy renameat2(2) RENAME_* flags are conceptually also
> syscall-specific
> + * flags, so it could makes sense to create the AT_RENAME_* aliases
> for them and
> + * maybe later add support for generic AT_* flags to this syscall.
> + * However, following a mismatch of definitions in glibc and since no
> kernel code
> + * currently uses the AT_RENAME_* aliases, we leave them undefined here.
> +#define AT_RENAME_NOREPLACE    RENAME_NOREPLACE
> +#define AT_RENAME_EXCHANGE     RENAME_EXCHANGE
> +#define AT_RENAME_WHITEOUT     RENAME_WHITEOUT
> +*/

Well, we do have samples/ code that uses fcntl.h (indirectly; maybe
that can be fixed).
See the build errors in the patch description.


>  /* Flag for faccessat(2). */
>  #define AT_EACCESS             0x200   /* Test access permitted for

With this patch (your suggestion above):

IF a userspace program in samples/ uses <uapi/linux/fcntl.h> without
using <stdio.h>, [yes, I created one to test this] and without using
<uapi/linux/fs.h> then the build fails with similar build errors:

../samples/watch_queue/watch_nostdio.c: In function ‘consumer’:
../samples/watch_queue/watch_nostdio.c:33:32: error: ‘RENAME_NOREPLACE’ undeclared (first use in this function)
   33 |                         return RENAME_NOREPLACE;
../samples/watch_queue/watch_nostdio.c:33:32: note: each undeclared identifier is reported only once for each function it appears in
../samples/watch_queue/watch_nostdio.c:37:32: error: ‘RENAME_EXCHANGE’ undeclared (first use in this function)
   37 |                         return RENAME_EXCHANGE;
../samples/watch_queue/watch_nostdio.c:41:32: error: ‘RENAME_WHITEOUT’ undeclared (first use in this function)
   41 |                         return RENAME_WHITEOUT;

This build succeeds with my version 1 patch (full defining of both
RENAME_* and AT_RENAME_* macros). It fails with the patch that you suggested
above.

OK, here's what I propose.

a. remove the unused and (sort of) recently added AT_RENAME_* macros
in include/uapi/linux/fcntl.h. Nothing in the kernel tree uses them.
This is:

commit b4fef22c2fb9
Author: Aleksa Sarai <cyphar@cyphar.com>
Date:   Wed Aug 28 20:19:42 2024 +1000
    uapi: explain how per-syscall AT_* flags should be allocated

These macros should have never been added here IMO.
Just putting them somewhere as examples (in comments) would be OK.

This alone fixes all of the build errors in samples/ that I originally
reported.

b. if a userspace program wants to use the RENAME_* macros, it should
#include <linux/fs.h> instead of <linux/fcntl.h>.

This fixes the "contrived" build error that I manufactured.

Note that some programs in tools/ do use AT_RENAME_* (all 3 macros)
but they define those macros locally.

-- 
~Randy