* [bug report] devlink: Move devlink dev reload code to dev
@ 2025-11-19 8:55 Dan Carpenter
2025-11-19 17:19 ` Jiri Pirko
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-11-19 8:55 UTC (permalink / raw)
To: Moshe Shemesh; +Cc: netdev
Hello Moshe Shemesh,
Commit c6ed7d6ef929 ("devlink: Move devlink dev reload code to dev")
from Feb 2, 2023 (linux-next), leads to the following Smatch static
checker warning:
net/devlink/dev.c:408 devlink_netns_get()
error: potential NULL/IS_ERR bug 'net'
net/devlink/dev.c
378 static struct net *devlink_netns_get(struct sk_buff *skb,
379 struct genl_info *info)
380 {
381 struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
382 struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
383 struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
384 struct net *net;
385
386 if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
387 NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
388 return ERR_PTR(-EINVAL);
389 }
390
391 if (netns_pid_attr) {
392 net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
Smatch thinks that the "net = get_net(nsproxy->net_ns);" could mean that
get_net_ns_by_pid() returns NULL. I don't know if that's correct or not.
If someone could tell me, then it's easy for me to add a line
"get_net_ns_by_pid 0" to the smatch_data/db/kernel.delete.return_states
file but I'd prefer to be sure before I do that...
393 } else if (netns_fd_attr) {
394 net = get_net_ns_by_fd(nla_get_u32(netns_fd_attr));
395 } else if (netns_id_attr) {
396 net = get_net_ns_by_id(sock_net(skb->sk),
397 nla_get_u32(netns_id_attr));
398 if (!net)
399 net = ERR_PTR(-EINVAL);
400 } else {
401 WARN_ON(1);
402 net = ERR_PTR(-EINVAL);
403 }
404 if (IS_ERR(net)) {
405 NL_SET_ERR_MSG(info->extack, "Unknown network namespace");
406 return ERR_PTR(-EINVAL);
407 }
--> 408 if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
409 put_net(net);
410 return ERR_PTR(-EPERM);
411 }
412 return net;
413 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] devlink: Move devlink dev reload code to dev
2025-11-19 8:55 [bug report] devlink: Move devlink dev reload code to dev Dan Carpenter
@ 2025-11-19 17:19 ` Jiri Pirko
2025-11-19 18:15 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Pirko @ 2025-11-19 17:19 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Moshe Shemesh, netdev
Wed, Nov 19, 2025 at 09:55:58AM +0100, dan.carpenter@linaro.org wrote:
>Hello Moshe Shemesh,
>
>Commit c6ed7d6ef929 ("devlink: Move devlink dev reload code to dev")
>from Feb 2, 2023 (linux-next), leads to the following Smatch static
>checker warning:
>
> net/devlink/dev.c:408 devlink_netns_get()
> error: potential NULL/IS_ERR bug 'net'
>
>net/devlink/dev.c
> 378 static struct net *devlink_netns_get(struct sk_buff *skb,
> 379 struct genl_info *info)
> 380 {
> 381 struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
> 382 struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
> 383 struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
> 384 struct net *net;
> 385
> 386 if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
> 387 NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
> 388 return ERR_PTR(-EINVAL);
> 389 }
> 390
> 391 if (netns_pid_attr) {
> 392 net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
>
>Smatch thinks that the "net = get_net(nsproxy->net_ns);" could mean that
>get_net_ns_by_pid() returns NULL. I don't know if that's correct or not.
>If someone could tell me, then it's easy for me to add a line
>"get_net_ns_by_pid 0" to the smatch_data/db/kernel.delete.return_states
>file but I'd prefer to be sure before I do that...
I don't see how get_net() can return NULL.
>
> 393 } else if (netns_fd_attr) {
> 394 net = get_net_ns_by_fd(nla_get_u32(netns_fd_attr));
> 395 } else if (netns_id_attr) {
> 396 net = get_net_ns_by_id(sock_net(skb->sk),
> 397 nla_get_u32(netns_id_attr));
> 398 if (!net)
> 399 net = ERR_PTR(-EINVAL);
> 400 } else {
> 401 WARN_ON(1);
> 402 net = ERR_PTR(-EINVAL);
> 403 }
> 404 if (IS_ERR(net)) {
> 405 NL_SET_ERR_MSG(info->extack, "Unknown network namespace");
> 406 return ERR_PTR(-EINVAL);
> 407 }
>--> 408 if (!netlink_ns_capable(skb, net->user_ns, CAP_NET_ADMIN)) {
> 409 put_net(net);
> 410 return ERR_PTR(-EPERM);
> 411 }
> 412 return net;
> 413 }
>
>regards,
>dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] devlink: Move devlink dev reload code to dev
2025-11-19 17:19 ` Jiri Pirko
@ 2025-11-19 18:15 ` Dan Carpenter
2025-11-20 12:10 ` Jiri Pirko
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2025-11-19 18:15 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Moshe Shemesh, netdev
On Wed, Nov 19, 2025 at 06:19:18PM +0100, Jiri Pirko wrote:
> Wed, Nov 19, 2025 at 09:55:58AM +0100, dan.carpenter@linaro.org wrote:
> >Hello Moshe Shemesh,
> >
> >Commit c6ed7d6ef929 ("devlink: Move devlink dev reload code to dev")
> >from Feb 2, 2023 (linux-next), leads to the following Smatch static
> >checker warning:
> >
> > net/devlink/dev.c:408 devlink_netns_get()
> > error: potential NULL/IS_ERR bug 'net'
> >
> >net/devlink/dev.c
> > 378 static struct net *devlink_netns_get(struct sk_buff *skb,
> > 379 struct genl_info *info)
> > 380 {
> > 381 struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
> > 382 struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
> > 383 struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
> > 384 struct net *net;
> > 385
> > 386 if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
> > 387 NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
> > 388 return ERR_PTR(-EINVAL);
> > 389 }
> > 390
> > 391 if (netns_pid_attr) {
> > 392 net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
> >
> >Smatch thinks that the "net = get_net(nsproxy->net_ns);" could mean that
> >get_net_ns_by_pid() returns NULL. I don't know if that's correct or not.
> >If someone could tell me, then it's easy for me to add a line
> >"get_net_ns_by_pid 0" to the smatch_data/db/kernel.delete.return_states
> >file but I'd prefer to be sure before I do that...
>
> I don't see how get_net() can return NULL.
>
It returns whatever you pass to it. The ns_ref_inc() macro
has a NULL check built in so it accepts NULL pointers.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] devlink: Move devlink dev reload code to dev
2025-11-19 18:15 ` Dan Carpenter
@ 2025-11-20 12:10 ` Jiri Pirko
2025-11-20 12:35 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Jiri Pirko @ 2025-11-20 12:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Moshe Shemesh, netdev
Wed, Nov 19, 2025 at 07:15:52PM +0100, dan.carpenter@linaro.org wrote:
>On Wed, Nov 19, 2025 at 06:19:18PM +0100, Jiri Pirko wrote:
>> Wed, Nov 19, 2025 at 09:55:58AM +0100, dan.carpenter@linaro.org wrote:
>> >Hello Moshe Shemesh,
>> >
>> >Commit c6ed7d6ef929 ("devlink: Move devlink dev reload code to dev")
>> >from Feb 2, 2023 (linux-next), leads to the following Smatch static
>> >checker warning:
>> >
>> > net/devlink/dev.c:408 devlink_netns_get()
>> > error: potential NULL/IS_ERR bug 'net'
>> >
>> >net/devlink/dev.c
>> > 378 static struct net *devlink_netns_get(struct sk_buff *skb,
>> > 379 struct genl_info *info)
>> > 380 {
>> > 381 struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
>> > 382 struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
>> > 383 struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
>> > 384 struct net *net;
>> > 385
>> > 386 if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
>> > 387 NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
>> > 388 return ERR_PTR(-EINVAL);
>> > 389 }
>> > 390
>> > 391 if (netns_pid_attr) {
>> > 392 net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
>> >
>> >Smatch thinks that the "net = get_net(nsproxy->net_ns);" could mean that
>> >get_net_ns_by_pid() returns NULL. I don't know if that's correct or not.
>> >If someone could tell me, then it's easy for me to add a line
>> >"get_net_ns_by_pid 0" to the smatch_data/db/kernel.delete.return_states
>> >file but I'd prefer to be sure before I do that...
>>
>> I don't see how get_net() can return NULL.
>>
>
>It returns whatever you pass to it. The ns_ref_inc() macro
>has a NULL check built in so it accepts NULL pointers.
Of course, I don't see how NULL can be passed to in in the current code.
>
>regards,
>dan carpenter
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [bug report] devlink: Move devlink dev reload code to dev
2025-11-20 12:10 ` Jiri Pirko
@ 2025-11-20 12:35 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2025-11-20 12:35 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Moshe Shemesh, netdev
On Thu, Nov 20, 2025 at 01:10:39PM +0100, Jiri Pirko wrote:
> Wed, Nov 19, 2025 at 07:15:52PM +0100, dan.carpenter@linaro.org wrote:
> >On Wed, Nov 19, 2025 at 06:19:18PM +0100, Jiri Pirko wrote:
> >> Wed, Nov 19, 2025 at 09:55:58AM +0100, dan.carpenter@linaro.org wrote:
> >> >Hello Moshe Shemesh,
> >> >
> >> >Commit c6ed7d6ef929 ("devlink: Move devlink dev reload code to dev")
> >> >from Feb 2, 2023 (linux-next), leads to the following Smatch static
> >> >checker warning:
> >> >
> >> > net/devlink/dev.c:408 devlink_netns_get()
> >> > error: potential NULL/IS_ERR bug 'net'
> >> >
> >> >net/devlink/dev.c
> >> > 378 static struct net *devlink_netns_get(struct sk_buff *skb,
> >> > 379 struct genl_info *info)
> >> > 380 {
> >> > 381 struct nlattr *netns_pid_attr = info->attrs[DEVLINK_ATTR_NETNS_PID];
> >> > 382 struct nlattr *netns_fd_attr = info->attrs[DEVLINK_ATTR_NETNS_FD];
> >> > 383 struct nlattr *netns_id_attr = info->attrs[DEVLINK_ATTR_NETNS_ID];
> >> > 384 struct net *net;
> >> > 385
> >> > 386 if (!!netns_pid_attr + !!netns_fd_attr + !!netns_id_attr > 1) {
> >> > 387 NL_SET_ERR_MSG(info->extack, "multiple netns identifying attributes specified");
> >> > 388 return ERR_PTR(-EINVAL);
> >> > 389 }
> >> > 390
> >> > 391 if (netns_pid_attr) {
> >> > 392 net = get_net_ns_by_pid(nla_get_u32(netns_pid_attr));
> >> >
> >> >Smatch thinks that the "net = get_net(nsproxy->net_ns);" could mean that
> >> >get_net_ns_by_pid() returns NULL. I don't know if that's correct or not.
> >> >If someone could tell me, then it's easy for me to add a line
> >> >"get_net_ns_by_pid 0" to the smatch_data/db/kernel.delete.return_states
> >> >file but I'd prefer to be sure before I do that...
> >>
> >> I don't see how get_net() can return NULL.
> >>
> >
> >It returns whatever you pass to it. The ns_ref_inc() macro
> >has a NULL check built in so it accepts NULL pointers.
>
> Of course, I don't see how NULL can be passed to in in the current code.
>
Fair enough... This is a complicated thing to track with static analysis
because create_new_namespaces() has a bit of recursion as far a Smatch is
concerned.
new_nsp->net_ns = copy_net_ns(flags, user_ns, tsk->nsproxy->net_ns);
It's copying the old tsk->nsproxy->net_ns to new_nsp->net_ns and Smatch
says if the old one is NULL then the new one could be NULL too. It's
just a bit complicated is all.
Anyway, it's a oneliner fix in Smatch.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-11-20 12:35 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-19 8:55 [bug report] devlink: Move devlink dev reload code to dev Dan Carpenter
2025-11-19 17:19 ` Jiri Pirko
2025-11-19 18:15 ` Dan Carpenter
2025-11-20 12:10 ` Jiri Pirko
2025-11-20 12:35 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox