* [bug report] netdevsim: fib: Add dummy implementation for FIB offload
@ 2020-01-20 14:37 Dan Carpenter
2020-01-20 14:43 ` Ido Schimmel
2020-01-20 15:17 ` Dan Carpenter
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-01-20 14:37 UTC (permalink / raw)
To: kernel-janitors
Hello Ido Schimmel,
The patch 48bb9eb47b27: "netdevsim: fib: Add dummy implementation for
FIB offload" from Jan 14, 2020, leads to the following static checker
warning:
drivers/net/netdevsim/fib.c:663 nsim_fib6_rt_insert()
error: 'fib6_rt' dereferencing possible ERR_PTR()
drivers/net/netdevsim/fib.c
460 nsim_fib6_rt_create(struct nsim_fib_data *data,
461 struct fib6_entry_notifier_info *fen6_info)
462 {
463 struct fib6_info *iter, *rt = fen6_info->rt;
464 struct nsim_fib6_rt *fib6_rt;
465 int i = 0;
466 int err;
467
468 fib6_rt = kzalloc(sizeof(*fib6_rt), GFP_ATOMIC);
469 if (!fib6_rt)
470 return NULL;
^^^^^^^^^^^
471
472 nsim_fib_rt_init(data, &fib6_rt->common, &rt->fib6_dst.addr,
473 sizeof(rt->fib6_dst.addr), rt->fib6_dst.plen, AF_INET6,
474 rt->fib6_table->tb6_id);
475
476 /* We consider a multipath IPv6 route as one entry, but it can be made
477 * up from several fib6_info structs (one for each nexthop), so we
478 * add them all to the same list under the entry.
479 */
480 INIT_LIST_HEAD(&fib6_rt->nh_list);
481
482 err = nsim_fib6_rt_nh_add(fib6_rt, rt);
483 if (err)
484 goto err_fib_rt_fini;
485
486 if (!fen6_info->nsiblings)
487 return fib6_rt;
488
489 list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {
490 if (i = fen6_info->nsiblings)
491 break;
492
493 err = nsim_fib6_rt_nh_add(fib6_rt, iter);
494 if (err)
495 goto err_fib6_rt_nh_del;
496 i++;
497 }
498 WARN_ON_ONCE(i != fen6_info->nsiblings);
499
500 return fib6_rt;
501
502 err_fib6_rt_nh_del:
503 list_for_each_entry_continue_reverse(iter, &rt->fib6_siblings,
504 fib6_siblings)
505 nsim_fib6_rt_nh_del(fib6_rt, iter);
506 nsim_fib6_rt_nh_del(fib6_rt, rt);
507 err_fib_rt_fini:
508 nsim_fib_rt_fini(&fib6_rt->common);
509 kfree(fib6_rt);
510 return ERR_PTR(err);
^^^^^^^^^^^^
511 }
This function should either return NULL or error pointers but not both.
When a function returns a mix of error pointers and NULL, the NULL case
should be a special kind of success. For example, we request a feature
like debugfs but it's disabled. That's not an error, but we also can't
return a valid pointer to the debug fs file.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] netdevsim: fib: Add dummy implementation for FIB offload
2020-01-20 14:37 [bug report] netdevsim: fib: Add dummy implementation for FIB offload Dan Carpenter
@ 2020-01-20 14:43 ` Ido Schimmel
2020-01-20 15:17 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Ido Schimmel @ 2020-01-20 14:43 UTC (permalink / raw)
To: kernel-janitors
On Mon, Jan 20, 2020 at 05:37:12PM +0300, Dan Carpenter wrote:
> Hello Ido Schimmel,
>
> The patch 48bb9eb47b27: "netdevsim: fib: Add dummy implementation for
> FIB offload" from Jan 14, 2020, leads to the following static checker
> warning:
>
> drivers/net/netdevsim/fib.c:663 nsim_fib6_rt_insert()
> error: 'fib6_rt' dereferencing possible ERR_PTR()
>
> drivers/net/netdevsim/fib.c
> 460 nsim_fib6_rt_create(struct nsim_fib_data *data,
> 461 struct fib6_entry_notifier_info *fen6_info)
> 462 {
> 463 struct fib6_info *iter, *rt = fen6_info->rt;
> 464 struct nsim_fib6_rt *fib6_rt;
> 465 int i = 0;
> 466 int err;
> 467
> 468 fib6_rt = kzalloc(sizeof(*fib6_rt), GFP_ATOMIC);
> 469 if (!fib6_rt)
> 470 return NULL;
> ^^^^^^^^^^^
Dan, thank you very much for the report. It is already fixed in net-next
thanks to Eric Dumazet:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=41cdc741048b0d04604c02aad9ec19f7d9130b70
Can you share the static checker you used so I can avoid these mistakes
in the future? Alternatively, is it possible to register development
trees for this service like with the kbuild robot?
Thanks in advance.
>
> 471
> 472 nsim_fib_rt_init(data, &fib6_rt->common, &rt->fib6_dst.addr,
> 473 sizeof(rt->fib6_dst.addr), rt->fib6_dst.plen, AF_INET6,
> 474 rt->fib6_table->tb6_id);
> 475
> 476 /* We consider a multipath IPv6 route as one entry, but it can be made
> 477 * up from several fib6_info structs (one for each nexthop), so we
> 478 * add them all to the same list under the entry.
> 479 */
> 480 INIT_LIST_HEAD(&fib6_rt->nh_list);
> 481
> 482 err = nsim_fib6_rt_nh_add(fib6_rt, rt);
> 483 if (err)
> 484 goto err_fib_rt_fini;
> 485
> 486 if (!fen6_info->nsiblings)
> 487 return fib6_rt;
> 488
> 489 list_for_each_entry(iter, &rt->fib6_siblings, fib6_siblings) {
> 490 if (i == fen6_info->nsiblings)
> 491 break;
> 492
> 493 err = nsim_fib6_rt_nh_add(fib6_rt, iter);
> 494 if (err)
> 495 goto err_fib6_rt_nh_del;
> 496 i++;
> 497 }
> 498 WARN_ON_ONCE(i != fen6_info->nsiblings);
> 499
> 500 return fib6_rt;
> 501
> 502 err_fib6_rt_nh_del:
> 503 list_for_each_entry_continue_reverse(iter, &rt->fib6_siblings,
> 504 fib6_siblings)
> 505 nsim_fib6_rt_nh_del(fib6_rt, iter);
> 506 nsim_fib6_rt_nh_del(fib6_rt, rt);
> 507 err_fib_rt_fini:
> 508 nsim_fib_rt_fini(&fib6_rt->common);
> 509 kfree(fib6_rt);
> 510 return ERR_PTR(err);
> ^^^^^^^^^^^^
> 511 }
>
> This function should either return NULL or error pointers but not both.
>
> When a function returns a mix of error pointers and NULL, the NULL case
> should be a special kind of success. For example, we request a feature
> like debugfs but it's disabled. That's not an error, but we also can't
> return a valid pointer to the debug fs file.
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [bug report] netdevsim: fib: Add dummy implementation for FIB offload
2020-01-20 14:37 [bug report] netdevsim: fib: Add dummy implementation for FIB offload Dan Carpenter
2020-01-20 14:43 ` Ido Schimmel
@ 2020-01-20 15:17 ` Dan Carpenter
1 sibling, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2020-01-20 15:17 UTC (permalink / raw)
To: kernel-janitors
On Mon, Jan 20, 2020 at 02:43:45PM +0000, Ido Schimmel wrote:
> On Mon, Jan 20, 2020 at 05:37:12PM +0300, Dan Carpenter wrote:
> > Hello Ido Schimmel,
> >
> > The patch 48bb9eb47b27: "netdevsim: fib: Add dummy implementation for
> > FIB offload" from Jan 14, 2020, leads to the following static checker
> > warning:
> >
> > drivers/net/netdevsim/fib.c:663 nsim_fib6_rt_insert()
> > error: 'fib6_rt' dereferencing possible ERR_PTR()
> >
> > drivers/net/netdevsim/fib.c
> > 460 nsim_fib6_rt_create(struct nsim_fib_data *data,
> > 461 struct fib6_entry_notifier_info *fen6_info)
> > 462 {
> > 463 struct fib6_info *iter, *rt = fen6_info->rt;
> > 464 struct nsim_fib6_rt *fib6_rt;
> > 465 int i = 0;
> > 466 int err;
> > 467
> > 468 fib6_rt = kzalloc(sizeof(*fib6_rt), GFP_ATOMIC);
> > 469 if (!fib6_rt)
> > 470 return NULL;
> > ^^^^^^^^^^^
>
> Dan, thank you very much for the report. It is already fixed in net-next
> thanks to Eric Dumazet:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?idAcdc741048b0d04604c02aad9ec19f7d9130b70
>
> Can you share the static checker you used so I can avoid these mistakes
> in the future? Alternatively, is it possible to register development
> trees for this service like with the kbuild robot?
>
> Thanks in advance.
It's a Smatch warning but you need to rebuild the kernel data.
~/path/to/smatch/smatch_scripts/build_kernel_data.sh
It takes a few hours (maybe four) to rebuild.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-01-20 15:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-20 14:37 [bug report] netdevsim: fib: Add dummy implementation for FIB offload Dan Carpenter
2020-01-20 14:43 ` Ido Schimmel
2020-01-20 15:17 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.