* re: ipv6: Do not depend on rt->n in rt6_probe().
@ 2013-01-21 18:28 Dan Carpenter
2013-01-21 18:41 ` YOSHIFUJI Hideaki
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2013-01-21 18:28 UTC (permalink / raw)
To: yoshfuji; +Cc: netdev, kbuild
Hello YOSHIFUJI Hideaki / 吉藤英明,
This is a semi-automatic email about new static checker warnings.
The patch 2152caea7196: "ipv6: Do not depend on rt->n in
rt6_probe()." from Jan 17, 2013, leads to the following Smatch
complaint:
net/ipv6/route.c:495 rt6_probe()
error: we previously assumed 'neigh' could be null (see line 490)
net/ipv6/route.c
489
490 if (!neigh ||
^^^^^
New test.
491 time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) {
492 struct in6_addr mcaddr;
493 struct in6_addr *target;
494
495 neigh->updated = jiffies;
^^^^^^^^^^^^^^
Old dereference.
496
497 if (neigh)
^^^^^
Another new test.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: ipv6: Do not depend on rt->n in rt6_probe(). 2013-01-21 18:28 ipv6: Do not depend on rt->n in rt6_probe() Dan Carpenter @ 2013-01-21 18:41 ` YOSHIFUJI Hideaki 2013-01-21 19:28 ` Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) YOSHIFUJI Hideaki 0 siblings, 1 reply; 6+ messages in thread From: YOSHIFUJI Hideaki @ 2013-01-21 18:41 UTC (permalink / raw) To: Dan Carpenter; +Cc: netdev, kbuild, YOSHIFUJI Hideaki (2013年01月22日 03:28), Dan Carpenter wrote: > Hello YOSHIFUJI Hideaki / 吉藤英明, > > This is a semi-automatic email about new static checker warnings. > > The patch 2152caea7196: "ipv6: Do not depend on rt->n in > rt6_probe()." from Jan 17, 2013, leads to the following Smatch > complaint: > > net/ipv6/route.c:495 rt6_probe() > error: we previously assumed 'neigh' could be null (see line 490) > > net/ipv6/route.c > 489 > 490 if (!neigh || > ^^^^^ > New test. > > 491 time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { > 492 struct in6_addr mcaddr; > 493 struct in6_addr *target; > 494 > 495 neigh->updated = jiffies; > ^^^^^^^^^^^^^^ > Old dereference. > > 496 > 497 if (neigh) > ^^^^^ > Another new test. Oh, right, I'll fix. Thanks! --yoshfuji ^ permalink raw reply [flat|nested] 6+ messages in thread
* Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) 2013-01-21 18:41 ` YOSHIFUJI Hideaki @ 2013-01-21 19:28 ` YOSHIFUJI Hideaki 2013-01-21 20:44 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: YOSHIFUJI Hideaki @ 2013-01-21 19:28 UTC (permalink / raw) To: David Miller; +Cc: YOSHIFUJI Hideaki, netdev (2013年01月22日 03:41), YOSHIFUJI Hideaki wrote: > (2013年01月22日 03:28), Dan Carpenter wrote: >> Hello YOSHIFUJI Hideaki / 吉藤英明, >> >> This is a semi-automatic email about new static checker warnings. >> >> The patch 2152caea7196: "ipv6: Do not depend on rt->n in >> rt6_probe()." from Jan 17, 2013, leads to the following Smatch >> complaint: >> >> net/ipv6/route.c:495 rt6_probe() >> error: we previously assumed 'neigh' could be null (see line 490) >> >> net/ipv6/route.c >> 489 >> 490 if (!neigh || >> ^^^^^ >> New test. >> >> 491 time_after(jiffies, neigh->updated + rt->rt6i_idev->cnf.rtr_probe_interval)) { >> 492 struct in6_addr mcaddr; >> 493 struct in6_addr *target; >> 494 >> 495 neigh->updated = jiffies; >> ^^^^^^^^^^^^^^ >> Old dereference. >> >> 496 >> 497 if (neigh) >> ^^^^^ >> Another new test. > > Oh, right, I'll fix. Thanks! Ok, fix is easy, but in fact, we have broken router reachability probing. Here rt->n was neighbour entry for (unreachable) router. The specification says, we SHOUDLD probe such router, but we should have some rate limit (once per minute, or so). We used "rt->n->updated" for this purpose, but now, if NS failed, we may immediately removes neighbour entry for it. So, we might continue sending NS to dead router every 1 second. Any ideas? --yoshfuji ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) 2013-01-21 19:28 ` Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) YOSHIFUJI Hideaki @ 2013-01-21 20:44 ` David Miller 2013-01-22 3:47 ` YOSHIFUJI Hideaki 0 siblings, 1 reply; 6+ messages in thread From: David Miller @ 2013-01-21 20:44 UTC (permalink / raw) To: yoshfuji; +Cc: netdev From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> Date: Tue, 22 Jan 2013 04:28:36 +0900 > Ok, fix is easy, but in fact, we have broken router reachability > probing. > > Here rt->n was neighbour entry for (unreachable) router. > The specification says, we SHOUDLD probe such router, but we > should have some rate limit (once per minute, or so). > > We used "rt->n->updated" for this purpose, but now, if NS failed, > we may immediately removes neighbour entry for it. So, > we might continue sending NS to dead router every 1 second. > > Any ideas? I don't see exactly how looking up the neigh on demand is different from using a cached one in this context. In both cases there should be a neigh entry in nd_tbl, why would there not be one? If necessary, you can decide to mark entries in such a way that they would have a lower priority for neigh GC purging if that is the issue. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) 2013-01-21 20:44 ` David Miller @ 2013-01-22 3:47 ` YOSHIFUJI Hideaki 2013-01-22 5:49 ` David Miller 0 siblings, 1 reply; 6+ messages in thread From: YOSHIFUJI Hideaki @ 2013-01-22 3:47 UTC (permalink / raw) To: David Miller; +Cc: netdev, YOSHIFUJI Hideaki David Miller wrote: > From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> > Date: Tue, 22 Jan 2013 04:28:36 +0900 > >> Ok, fix is easy, but in fact, we have broken router reachability >> probing. >> >> Here rt->n was neighbour entry for (unreachable) router. >> The specification says, we SHOUDLD probe such router, but we >> should have some rate limit (once per minute, or so). >> >> We used "rt->n->updated" for this purpose, but now, if NS failed, >> we may immediately removes neighbour entry for it. So, >> we might continue sending NS to dead router every 1 second. >> >> Any ideas? > > I don't see exactly how looking up the neigh on demand is different > from using a cached one in this context. > > In both cases there should be a neigh entry in nd_tbl, why would > there not be one? Well, now, the almost only refcnt holder except table is timer. because each route does not have reference to neighbour (thus no refcnt) anymore. If n->nud_state become NUD_FAILED (or even in NUD_STALE), the entry does not have any refcnt holders other than table, and then, neigh_periodic_work() will purge such entries. > If necessary, you can decide to mark entries in such a way that > they would have a lower priority for neigh GC purging if that > is the issue. It seems that we removed check for gc_thresh1 (number of minimum entries) in neigh_alloc() during 2.3.x but I cannot remember the reason (or just I do not know it). --yoshfuji ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) 2013-01-22 3:47 ` YOSHIFUJI Hideaki @ 2013-01-22 5:49 ` David Miller 0 siblings, 0 replies; 6+ messages in thread From: David Miller @ 2013-01-22 5:49 UTC (permalink / raw) To: yoshfuji; +Cc: netdev From: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org> Date: Tue, 22 Jan 2013 12:47:42 +0900 > It seems that we removed check for gc_thresh1 (number of > minimum entries) in neigh_alloc() during 2.3.x but I cannot > remember the reason (or just I do not know it). You seem to suggest that neigh_periodic_work() should check gc_thresh1 and I agree with this idea. Considering how neighbour references work now, what it does currently doesn't make any sense. Right now it runs over and over again, unconditionally, and purges entries even if there are very few of them. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-01-22 5:49 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-01-21 18:28 ipv6: Do not depend on rt->n in rt6_probe() Dan Carpenter 2013-01-21 18:41 ` YOSHIFUJI Hideaki 2013-01-21 19:28 ` Rate Limitation of Router Reachability Probing for possible dead routers (is Re: ipv6: Do not depend on rt->n in rt6_probe().) YOSHIFUJI Hideaki 2013-01-21 20:44 ` David Miller 2013-01-22 3:47 ` YOSHIFUJI Hideaki 2013-01-22 5:49 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).