[PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[Jens Axboe]

Add compat.h include to avoid a potential build issue:

io_uring/register.c:281:6: error: call to undeclared function 'in_compat_syscall'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]

if (in_compat_syscall()) {
    ^
1 warning generated.
io_uring/register.c:282:9: error: call to undeclared function 'compat_get_bitmap'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
ret = compat_get_bitmap(cpumask_bits(new_mask),
      ^

Fixes: c43203154d8a ("io_uring/register: move io_uring_register(2) related code to register.c")
Reported-by: Manu Bretelle <chantra@meta.com>
Signed-off-by: Jens Axboe <axboe@kernel.dk>

---

diff --git a/io_uring/register.c b/io_uring/register.c
index 708dd1d89add..5e62c1208996 100644
--- a/io_uring/register.c
+++ b/io_uring/register.c
@@ -14,6 +14,7 @@
 #include <linux/slab.h>
 #include <linux/uaccess.h>
 #include <linux/nospec.h>
+#include <linux/compat.h>
 #include <linux/io_uring.h>
 #include <linux/io_uring_types.h>
 
@@ -278,13 +279,14 @@ static __cold int io_register_iowq_aff(struct io_ring_ctx *ctx,
 	if (len > cpumask_size())
 		len = cpumask_size();
 
-	if (in_compat_syscall()) {
+#ifdef CONFIG_COMPAT
+	if (in_compat_syscall())
 		ret = compat_get_bitmap(cpumask_bits(new_mask),
 					(const compat_ulong_t __user *)arg,
 					len * 8 /* CHAR_BIT */);
-	} else {
+	else
+#endif
 		ret = copy_from_user(new_mask, arg, len);
-	}
 
 	if (ret) {
 		free_cpumask_var(new_mask);

-- 
Jens Axboe

Re: [PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[Jeff Moyer]

Hi, Jens,

Jens Axboe <axboe@kernel.dk> writes:

> Add compat.h include to avoid a potential build issue:
>
> io_uring/register.c:281:6: error: call to undeclared function 'in_compat_syscall'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>
> if (in_compat_syscall()) {
>     ^
> 1 warning generated.
> io_uring/register.c:282:9: error: call to undeclared function 'compat_get_bitmap'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
> ret = compat_get_bitmap(cpumask_bits(new_mask),
>       ^
>
> Fixes: c43203154d8a ("io_uring/register: move io_uring_register(2) related code to register.c")
> Reported-by: Manu Bretelle <chantra@meta.com>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>
> ---
>
> diff --git a/io_uring/register.c b/io_uring/register.c
> index 708dd1d89add..5e62c1208996 100644
> --- a/io_uring/register.c
> +++ b/io_uring/register.c
> @@ -14,6 +14,7 @@
>  #include <linux/slab.h>
>  #include <linux/uaccess.h>
>  #include <linux/nospec.h>
> +#include <linux/compat.h>
>  #include <linux/io_uring.h>
>  #include <linux/io_uring_types.h>

This makes sense to me, but I wasn't able to reproduce that build error
after disabling CONFIG_COMPAT.

> @@ -278,13 +279,14 @@ static __cold int io_register_iowq_aff(struct io_ring_ctx *ctx,
>  	if (len > cpumask_size())
>  		len = cpumask_size();
>  
> -	if (in_compat_syscall()) {
> +#ifdef CONFIG_COMPAT
> +	if (in_compat_syscall())

I don't think this is needed.

linux/compat.h:
...
#else /* !CONFIG_COMPAT */

#define is_compat_task() (0)
/* Ensure no one redefines in_compat_syscall() under !CONFIG_COMPAT */
#define in_compat_syscall in_compat_syscall
static inline bool in_compat_syscall(void) { return false; }

Isn't the code fine as-is?

-Jeff

>  		ret = compat_get_bitmap(cpumask_bits(new_mask),
>  					(const compat_ulong_t __user *)arg,
>  					len * 8 /* CHAR_BIT */);
> -	} else {
> +	else
> +#endif
>  		ret = copy_from_user(new_mask, arg, len);
> -	}
>  
>  	if (ret) {
>  		free_cpumask_var(new_mask);

Re: [PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[Jens Axboe]

On 1/17/24 8:59 AM, Jeff Moyer wrote:
> Hi, Jens,
> 
> Jens Axboe <axboe@kernel.dk> writes:
> 
>> Add compat.h include to avoid a potential build issue:
>>
>> io_uring/register.c:281:6: error: call to undeclared function 'in_compat_syscall'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>>
>> if (in_compat_syscall()) {
>>     ^
>> 1 warning generated.
>> io_uring/register.c:282:9: error: call to undeclared function 'compat_get_bitmap'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>> ret = compat_get_bitmap(cpumask_bits(new_mask),
>>       ^
>>
>> Fixes: c43203154d8a ("io_uring/register: move io_uring_register(2) related code to register.c")
>> Reported-by: Manu Bretelle <chantra@meta.com>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>
>> ---
>>
>> diff --git a/io_uring/register.c b/io_uring/register.c
>> index 708dd1d89add..5e62c1208996 100644
>> --- a/io_uring/register.c
>> +++ b/io_uring/register.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/uaccess.h>
>>  #include <linux/nospec.h>
>> +#include <linux/compat.h>
>>  #include <linux/io_uring.h>
>>  #include <linux/io_uring_types.h>
> 
> This makes sense to me, but I wasn't able to reproduce that build error
> after disabling CONFIG_COMPAT.

I couldn't either, but apparently it happened internally with our kdump
config variant.

>> @@ -278,13 +279,14 @@ static __cold int io_register_iowq_aff(struct io_ring_ctx *ctx,
>>  	if (len > cpumask_size())
>>  		len = cpumask_size();
>>  
>> -	if (in_compat_syscall()) {
>> +#ifdef CONFIG_COMPAT
>> +	if (in_compat_syscall())
> 
> I don't think this is needed.
> 
> linux/compat.h:
> ...
> #else /* !CONFIG_COMPAT */
> 
> #define is_compat_task() (0)
> /* Ensure no one redefines in_compat_syscall() under !CONFIG_COMPAT */
> #define in_compat_syscall in_compat_syscall
> static inline bool in_compat_syscall(void) { return false; }
> 
> Isn't the code fine as-is?

It probably is, but this makes it consistent with the other spots we do
compat handling. Hence I'd prefer to keep it like that, and then perhaps
we can prune them all at some point.

Thanks for taking a look!

-- 
Jens Axboe

Re: [PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[Jeff Moyer]

Jens Axboe <axboe@kernel.dk> writes:

> On 1/17/24 8:59 AM, Jeff Moyer wrote:
>> Hi, Jens,
>> 
>> Jens Axboe <axboe@kernel.dk> writes:
>> 
>>> Add compat.h include to avoid a potential build issue:
>>>
>>> io_uring/register.c:281:6: error: call to undeclared function 'in_compat_syscall'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>>>
>>> if (in_compat_syscall()) {
>>>     ^
>>> 1 warning generated.
>>> io_uring/register.c:282:9: error: call to undeclared function 'compat_get_bitmap'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
>>> ret = compat_get_bitmap(cpumask_bits(new_mask),
>>>       ^
>>>
>>> Fixes: c43203154d8a ("io_uring/register: move io_uring_register(2) related code to register.c")
>>> Reported-by: Manu Bretelle <chantra@meta.com>
>>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> ---
>>>
>>> diff --git a/io_uring/register.c b/io_uring/register.c
>>> index 708dd1d89add..5e62c1208996 100644
>>> --- a/io_uring/register.c
>>> +++ b/io_uring/register.c
>>> @@ -14,6 +14,7 @@
>>>  #include <linux/slab.h>
>>>  #include <linux/uaccess.h>
>>>  #include <linux/nospec.h>
>>> +#include <linux/compat.h>
>>>  #include <linux/io_uring.h>
>>>  #include <linux/io_uring_types.h>
>> 
>> This makes sense to me, but I wasn't able to reproduce that build error
>> after disabling CONFIG_COMPAT.
>
> I couldn't either, but apparently it happened internally with our kdump
> config variant.

ok.

>>> @@ -278,13 +279,14 @@ static __cold int io_register_iowq_aff(struct io_ring_ctx *ctx,
>>>  	if (len > cpumask_size())
>>>  		len = cpumask_size();
>>>  
>>> -	if (in_compat_syscall()) {
>>> +#ifdef CONFIG_COMPAT
>>> +	if (in_compat_syscall())
>> 
>> I don't think this is needed.
>> 
>> linux/compat.h:
>> ...
>> #else /* !CONFIG_COMPAT */
>> 
>> #define is_compat_task() (0)
>> /* Ensure no one redefines in_compat_syscall() under !CONFIG_COMPAT */
>> #define in_compat_syscall in_compat_syscall
>> static inline bool in_compat_syscall(void) { return false; }
>> 
>> Isn't the code fine as-is?
>
> It probably is, but this makes it consistent with the other spots we do
> compat handling. Hence I'd prefer to keep it like that, and then perhaps
> we can prune them all at some point.

I see one other spot.  :)  But if you are happy with it, that's fine by
me.

> Thanks for taking a look!

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Cheers,
Jeff

Re: [PATCH] io_uring/register: guard compat syscall with CONFIG_COMPAT

[Jens Axboe]

On 1/17/24 9:24 AM, Jeff Moyer wrote:
>>>> @@ -278,13 +279,14 @@ static __cold int io_register_iowq_aff(struct io_ring_ctx *ctx,
>>>>  	if (len > cpumask_size())
>>>>  		len = cpumask_size();
>>>>  
>>>> -	if (in_compat_syscall()) {
>>>> +#ifdef CONFIG_COMPAT
>>>> +	if (in_compat_syscall())
>>>
>>> I don't think this is needed.
>>>
>>> linux/compat.h:
>>> ...
>>> #else /* !CONFIG_COMPAT */
>>>
>>> #define is_compat_task() (0)
>>> /* Ensure no one redefines in_compat_syscall() under !CONFIG_COMPAT */
>>> #define in_compat_syscall in_compat_syscall
>>> static inline bool in_compat_syscall(void) { return false; }
>>>
>>> Isn't the code fine as-is?
>>
>> It probably is, but this makes it consistent with the other spots we do
>> compat handling. Hence I'd prefer to keep it like that, and then perhaps
>> we can prune them all at some point.
> 
> I see one other spot.  :)  But if you are happy with it, that's fine by
> me.

I already shipped it internally like that, which is another reason why
I'd like to just keep it consistent and then we can do a cleanup on top
for 6.9 once tested.

>> Thanks for taking a look!
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks!

-- 
Jens Axboe