On Wed, Apr 28, 2021 at 02:56:27PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> On Wed, Apr 28, 2021 at 02:25:26AM +0200, Alexander Bluhm wrote:
> > On Wed, Apr 28, 2021 at 03:19:47AM +0300, Vitaliy Makkoveev wrote:
> > > The code not only breaks loop but also cleans the queue. Should this be
> > > kept?
> >
>
> this is a good point
>
> > In IPv6 nd6_cache_lladdr() we have neither queue nor loop, just a
> > single mbuf on hold. But we have that discard logic, too.
> >
> > if (ln->ln_hold) {
> > struct mbuf *n = ln->ln_hold;
> > ln->ln_hold = NULL;
> > /*
> > * we assume ifp is not a p2p here, so just
> > * set the 2nd argument as the 1st one.
> > */
> > ifp->if_output(ifp, n, rt_key(rt), rt);
> > if (ln->ln_hold == n) {
> > /* n is back in ln_hold. Discard. */
> > m_freem(ln->ln_hold);
> > ln->ln_hold = NULL;
> > }
> > }
> >
> > So I guess there is a reason to clean up.
> >
>
> I would say the la->la_mq queue should be empty once we dispatch all
> packets to wire. If it is not empty, then something went wrong and packets
> got recirculated back to la->la_mq. I think dropping them is better
> plan than keeping them in queue. There is not much we can do in this
> case than to drop them.
Another option is to assert on the condition. It is an error case
that should not exist.
There are a few more issues in the arp code once the concurrency goes up.
The way the link-local address is updated looks not save to me.
At least arpresolve() and arpcache() need to make sure that access to
rt_gateway is either serialized or the updates happen so that the result
is always correct. At the moment this is not the case.
I assume that arp_rtrequest() is accessed with an exclusive NET_LOCK if
not then there is more trouble waiting (setting rt_llinfo early is one
of them).
--
:wq Claudio
