| Message ID | 20250227132018.1111094-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring/net: fix build warning for !CONFIG_COMPAT | expand |
On 2/27/25 13:20, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > A code rework resulted in an uninitialized return code when COMPAT > mode is disabled: As mentioned in the lkp report, it should be a false positive. > > io_uring/net.c:722:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > 722 | if (io_is_compat(req->ctx)) { > | ^~~~~~~~~~~~~~~~~~~~~~ > io_uring/net.c:736:15: note: uninitialized use occurs here > 736 | if (unlikely(ret)) > | ^~~ > > Since io_is_compat() turns into a compile-time 'false', the #ifdef > here is completely unnecessary, and removing it avoids the warning. I don't think __get_compat_msghdr() and other helpers are compiled for !COMPAT. I'd just silence it like: if (io_is_compat(req->ctx)) { ret = -EFAULT; #ifdef CONFIG_COMPAT ... #endif CONFIG_COMPAT } Let's see if Jens wants to fix it up in the tree.
On Thu, Feb 27, 2025, at 14:49, Pavel Begunkov wrote: > On 2/27/25 13:20, Arnd Bergmann wrote: >> | ^~~ >> >> Since io_is_compat() turns into a compile-time 'false', the #ifdef >> here is completely unnecessary, and removing it avoids the warning. > > I don't think __get_compat_msghdr() and other helpers are > compiled for !COMPAT. They are not defined without CONFIG_COMPAT. My point in the message is that io_is_compat() turning into a compile-time 'false' value means that they also don't get called, because compilers are really good at this type of dead code elimination. > I'd just silence it like: > > if (io_is_compat(req->ctx)) { > ret = -EFAULT; > #ifdef CONFIG_COMPAT > ... > #endif CONFIG_COMPAT > } That seems even less readable. If you want to be explicit about it, you could use if (IS_ENABLED(CONFIG_COMPAT) && io_is_compat(req->ctx)) { to replace the #ifdef, but as I wrote in the patch description, the compile-time check is really redundant because io_is_compat() is meant to do exactly that. Arnd
On 2/27/25 13:49, Pavel Begunkov wrote: > On 2/27/25 13:20, Arnd Bergmann wrote: >> From: Arnd Bergmann <arnd@arndb.de> >> >> A code rework resulted in an uninitialized return code when COMPAT >> mode is disabled: > > As mentioned in the lkp report, it should be a false positive. > >> >> io_uring/net.c:722:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] >> 722 | if (io_is_compat(req->ctx)) { >> | ^~~~~~~~~~~~~~~~~~~~~~ >> io_uring/net.c:736:15: note: uninitialized use occurs here >> 736 | if (unlikely(ret)) >> | ^~~ >> >> Since io_is_compat() turns into a compile-time 'false', the #ifdef >> here is completely unnecessary, and removing it avoids the warning. > > I don't think __get_compat_msghdr() and other helpers are > compiled for !COMPAT. I'd just silence it like: I guess we're relying on dead code elimination to prevent linking against them, if that's a normal practise and we do mandate compilers to do that, then it looks fine to me > > if (io_is_compat(req->ctx)) { > ret = -EFAULT; > #ifdef CONFIG_COMPAT > ... > #endif CONFIG_COMPAT > } > > Let's see if Jens wants to fix it up in the tree. >
On Thu, 27 Feb 2025 14:20:09 +0100, Arnd Bergmann wrote: > A code rework resulted in an uninitialized return code when COMPAT > mode is disabled: > > io_uring/net.c:722:6: error: variable 'ret' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized] > 722 | if (io_is_compat(req->ctx)) { > | ^~~~~~~~~~~~~~~~~~~~~~ > io_uring/net.c:736:15: note: uninitialized use occurs here > 736 | if (unlikely(ret)) > | ^~~ > > [...] Applied, thanks! [1/1] io_uring/net: fix build warning for !CONFIG_COMPAT commit: 4afc332bc86c34b74f1211650f748feb6942a9cc Best regards,
On 2/27/25 13:58, Arnd Bergmann wrote: > On Thu, Feb 27, 2025, at 14:49, Pavel Begunkov wrote: >> On 2/27/25 13:20, Arnd Bergmann wrote: >>> | ^~~ >>> >>> Since io_is_compat() turns into a compile-time 'false', the #ifdef >>> here is completely unnecessary, and removing it avoids the warning. >> >> I don't think __get_compat_msghdr() and other helpers are >> compiled for !COMPAT. > > They are not defined without CONFIG_COMPAT. My point in the > message is that io_is_compat() turning into a compile-time > 'false' value means that they also don't get called, because > compilers are really good at this type of dead code elimination. > >> I'd just silence it like: >> >> if (io_is_compat(req->ctx)) { >> ret = -EFAULT; >> #ifdef CONFIG_COMPAT >> ... >> #endif CONFIG_COMPAT >> } > > That seems even less readable. If you want to be explicit > about it, you could use > > if (IS_ENABLED(CONFIG_COMPAT) && io_is_compat(req->ctx)) { > > to replace the #ifdef, but as I wrote in the patch > description, the compile-time check is really redundant > because io_is_compat() is meant to do exactly that. I do like getting rid of #ifdef, just a bit surprised that we're relying on dead code elimination, even though it's what any reasonable compiler must do. Btw, sth went wrong with the reply: (delivered after 24733 seconds)
diff --git a/io_uring/net.c b/io_uring/net.c index bb454b9f6a4c..33e9be094131 100644 --- a/io_uring/net.c +++ b/io_uring/net.c @@ -224,7 +224,6 @@ static int io_net_import_vec(struct io_kiocb *req, struct io_async_msghdr *iomsg return 0; } -#ifdef CONFIG_COMPAT static int io_compat_msg_copy_hdr(struct io_kiocb *req, struct io_async_msghdr *iomsg, struct compat_msghdr *msg, int ddir, @@ -263,7 +262,6 @@ static int io_compat_msg_copy_hdr(struct io_kiocb *req, return io_net_import_vec(req, iomsg, (struct iovec __user *)uiov, msg->msg_iovlen, ddir); } -#endif static int io_copy_msghdr_from_user(struct user_msghdr *msg, struct user_msghdr __user *umsg) @@ -330,7 +328,6 @@ static int io_sendmsg_copy_hdr(struct io_kiocb *req, iomsg->msg.msg_name = &iomsg->addr; iomsg->msg.msg_iter.nr_segs = 0; -#ifdef CONFIG_COMPAT if (io_is_compat(req->ctx)) { struct compat_msghdr cmsg; @@ -339,7 +336,6 @@ static int io_sendmsg_copy_hdr(struct io_kiocb *req, sr->msg_control = iomsg->msg.msg_control_user; return ret; } -#endif ret = io_msg_copy_hdr(req, iomsg, &msg, ITER_SOURCE, NULL); /* save msg_control as sys_sendmsg() overwrites it */ @@ -720,7 +716,6 @@ static int io_recvmsg_copy_hdr(struct io_kiocb *req, iomsg->msg.msg_iter.nr_segs = 0; if (io_is_compat(req->ctx)) { -#ifdef CONFIG_COMPAT struct compat_msghdr cmsg; ret = io_compat_msg_copy_hdr(req, iomsg, &cmsg, ITER_DEST, @@ -728,7 +723,6 @@ static int io_recvmsg_copy_hdr(struct io_kiocb *req, memset(&msg, 0, sizeof(msg)); msg.msg_namelen = cmsg.msg_namelen; msg.msg_controllen = cmsg.msg_controllen; -#endif } else { ret = io_msg_copy_hdr(req, iomsg, &msg, ITER_DEST, &iomsg->uaddr); }