On Wed, Apr 28, 2021 at 03:31:41PM +0200, Claudio Jeker wrote:
> 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.
>
I think assert won't hurt here as it will enable us to learn something.
I suspect we can increase a chance to trigger error condition
by doing 'arp -a -d'.
> 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).
>
this is the case currently as far as I can tell.
regards
sashan
