* [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE()
@ 2025-11-26 8:31 Dan Carpenter
2025-11-26 13:53 ` Chuck Lever
2025-11-26 15:36 ` Chuck Lever
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2025-11-26 8:31 UTC (permalink / raw)
To: Christian Brauner; +Cc: kernel-tls-handshake
Hello Christian Brauner,
Commit 214ab7edf554 ("net/handshake: convert
handshake_nl_accept_doit() to FD_PREPARE()") from Nov 23, 2025
(linux-next), leads to the following Smatch static checker warning:
net/handshake/netlink.c:128 handshake_nl_accept_doit()
error: we previously assumed 'req' could be null (see line 109)
net/handshake/netlink.c
90 int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
91 {
92 struct net *net = sock_net(skb->sk);
93 struct handshake_net *hn = handshake_pernet(net);
94 struct handshake_req *req = NULL;
95 struct socket *sock;
96 int class, err;
97
98 err = -EOPNOTSUPP;
99 if (!hn)
100 goto out_status;
101
102 err = -EINVAL;
103 if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_ACCEPT_HANDLER_CLASS))
104 goto out_status;
105 class = nla_get_u32(info->attrs[HANDSHAKE_A_ACCEPT_HANDLER_CLASS]);
106
107 err = -EAGAIN;
108 req = handshake_req_next(hn, class);
109 if (req) {
If handshake_req_next() returns NULL
110 sock = req->hr_sk->sk_socket;
111
112 FD_PREPARE(fdf, O_CLOEXEC, sock->file);
113 if (fdf.err) {
114 err = fdf.err;
115 goto out_complete;
116 }
117
118 get_file(sock->file); /* FD_PREPARE() consumes a reference. */
119 err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
120 if (err)
121 goto out_complete; /* Automatic cleanup handles fput */
122
123 trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
124 return fd_publish(fdf);
125 }
126
127 out_complete:
--> 128 handshake_complete(req, -EIO, NULL);
^^^
then this will crash.
129 out_status:
130 trace_handshake_cmd_accept_err(net, req, NULL, err);
131 return err;
132 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE()
2025-11-26 8:31 [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE() Dan Carpenter
@ 2025-11-26 13:53 ` Chuck Lever
2025-11-26 15:36 ` Chuck Lever
1 sibling, 0 replies; 3+ messages in thread
From: Chuck Lever @ 2025-11-26 13:53 UTC (permalink / raw)
To: Dan Carpenter, Christian Brauner; +Cc: kernel-tls-handshake
On Wed, Nov 26, 2025, at 3:31 AM, Dan Carpenter wrote:
> Hello Christian Brauner,
>
> Commit 214ab7edf554 ("net/handshake: convert
> handshake_nl_accept_doit() to FD_PREPARE()") from Nov 23, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
> net/handshake/netlink.c:128 handshake_nl_accept_doit()
> error: we previously assumed 'req' could be null (see line 109)
>
> net/handshake/netlink.c
> 90 int handshake_nl_accept_doit(struct sk_buff *skb, struct
> genl_info *info)
> 91 {
> 92 struct net *net = sock_net(skb->sk);
> 93 struct handshake_net *hn = handshake_pernet(net);
> 94 struct handshake_req *req = NULL;
> 95 struct socket *sock;
> 96 int class, err;
> 97
> 98 err = -EOPNOTSUPP;
> 99 if (!hn)
> 100 goto out_status;
> 101
> 102 err = -EINVAL;
> 103 if (GENL_REQ_ATTR_CHECK(info,
> HANDSHAKE_A_ACCEPT_HANDLER_CLASS))
> 104 goto out_status;
> 105 class =
> nla_get_u32(info->attrs[HANDSHAKE_A_ACCEPT_HANDLER_CLASS]);
> 106
> 107 err = -EAGAIN;
> 108 req = handshake_req_next(hn, class);
> 109 if (req) {
>
> If handshake_req_next() returns NULL
>
> 110 sock = req->hr_sk->sk_socket;
> 111
> 112 FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> 113 if (fdf.err) {
> 114 err = fdf.err;
> 115 goto out_complete;
> 116 }
> 117
> 118 get_file(sock->file); /* FD_PREPARE() consumes
> a reference. */
> 119 err = req->hr_proto->hp_accept(req, info,
> fd_prepare_fd(fdf));
> 120 if (err)
> 121 goto out_complete; /* Automatic cleanup
> handles fput */
> 122
> 123 trace_handshake_cmd_accept(net, req,
> req->hr_sk, fd_prepare_fd(fdf));
> 124 return fd_publish(fdf);
> 125 }
> 126
> 127 out_complete:
> --> 128 handshake_complete(req, -EIO, NULL);
> ^^^
> then this will crash.
>
> 129 out_status:
> 130 trace_handshake_cmd_accept_err(net, req, NULL, err);
> 131 return err;
> 132 }
>
> regards,
> dan carpenter
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/net/handshake/netlink.c?id=0f3d7fa5007a11d8a0eab88c970086363afb1564
I've searched the netdev@ archive on lore, and don't see this
commit. Neither is it in netdev-next.
I'm a little concerned that a change to net/handshake/netlink.c
was never sent to any of its listed maintainers for review or
Acked-by.
--
Chuck Lever
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE()
2025-11-26 8:31 [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE() Dan Carpenter
2025-11-26 13:53 ` Chuck Lever
@ 2025-11-26 15:36 ` Chuck Lever
1 sibling, 0 replies; 3+ messages in thread
From: Chuck Lever @ 2025-11-26 15:36 UTC (permalink / raw)
To: Christian Brauner; +Cc: kernel-tls-handshake, Dan Carpenter
On Wed, Nov 26, 2025 at 11:31:32AM +0300, Dan Carpenter wrote:
> Hello Christian Brauner,
>
> Commit 214ab7edf554 ("net/handshake: convert
> handshake_nl_accept_doit() to FD_PREPARE()") from Nov 23, 2025
> (linux-next), leads to the following Smatch static checker warning:
>
> net/handshake/netlink.c:128 handshake_nl_accept_doit()
> error: we previously assumed 'req' could be null (see line 109)
>
> net/handshake/netlink.c
> 90 int handshake_nl_accept_doit(struct sk_buff *skb, struct genl_info *info)
> 91 {
> 92 struct net *net = sock_net(skb->sk);
> 93 struct handshake_net *hn = handshake_pernet(net);
> 94 struct handshake_req *req = NULL;
> 95 struct socket *sock;
> 96 int class, err;
> 97
> 98 err = -EOPNOTSUPP;
> 99 if (!hn)
> 100 goto out_status;
> 101
> 102 err = -EINVAL;
> 103 if (GENL_REQ_ATTR_CHECK(info, HANDSHAKE_A_ACCEPT_HANDLER_CLASS))
> 104 goto out_status;
> 105 class = nla_get_u32(info->attrs[HANDSHAKE_A_ACCEPT_HANDLER_CLASS]);
> 106
> 107 err = -EAGAIN;
> 108 req = handshake_req_next(hn, class);
> 109 if (req) {
>
> If handshake_req_next() returns NULL
>
> 110 sock = req->hr_sk->sk_socket;
> 111
> 112 FD_PREPARE(fdf, O_CLOEXEC, sock->file);
> 113 if (fdf.err) {
> 114 err = fdf.err;
> 115 goto out_complete;
> 116 }
> 117
> 118 get_file(sock->file); /* FD_PREPARE() consumes a reference. */
> 119 err = req->hr_proto->hp_accept(req, info, fd_prepare_fd(fdf));
> 120 if (err)
> 121 goto out_complete; /* Automatic cleanup handles fput */
> 122
> 123 trace_handshake_cmd_accept(net, req, req->hr_sk, fd_prepare_fd(fdf));
> 124 return fd_publish(fdf);
> 125 }
> 126
> 127 out_complete:
> --> 128 handshake_complete(req, -EIO, NULL);
> ^^^
> then this will crash.
>
> 129 out_status:
> 130 trace_handshake_cmd_accept_err(net, req, NULL, err);
> 131 return err;
> 132 }
>
> regards,
> dan carpenter
>
Following up with a review comment or two, based on:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/net/handshake/netlink.c?id=0f3d7fa5007a11d8a0eab88c970086363afb1564
The original code followed the preferred early exit / early return
kernel style. The new version inverts this by wrapping the success
path inside:
if (req) { ... }
instead of checking
if (!req)
goto out;
and then continuing the main execution flow at the base indentation
level. Is there a reason the original style needs to be abandoned?
--
Chuck Lever
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-11-26 15:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-26 8:31 [bug report] net/handshake: convert handshake_nl_accept_doit() to FD_PREPARE() Dan Carpenter
2025-11-26 13:53 ` Chuck Lever
2025-11-26 15:36 ` Chuck Lever
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox