Re: [PATCH] neighbour: purge nf_bridged skb from foreign device neigh

[Pavel Tikhomirov]

On 08/01/2024 19:15, Florian Westphal wrote:
> Pavel Tikhomirov <ptikhomirov@xxxxxxxxxxxxx> wrote:
> > An skb can be added to a neigh->arp_queue while waiting for an arp
> > reply. Where original skb's skb->dev can be different to neigh's
> > neigh->dev. For instance in case of bridging dnated skb from one veth to
> > another, the skb would be added to a neigh->arp_queue of the bridge.
> >
> > There is no explicit mechanism that prevents the original skb->dev link
> > of such skb from being freed under us. For instance neigh_flush_dev does
> > not cleanup skbs from different device's neigh queue. But that original
> > link can be used and lead to crash on e.g. this stack:
> >
> > arp_process
> >    neigh_update
> >      skb = __skb_dequeue(&neigh->arp_queue)
> >        neigh_resolve_output(..., skb)
> >          ...
> >            br_nf_dev_xmit
> >              br_nf_pre_routing_finish_bridge_slow
> >                skb->dev = nf_bridge->physindev
> >                br_handle_frame_finish
> >
> > So let's improve neigh_flush_dev to also purge skbs when device
> > equal to their skb->nf_bridge->physindev gets destroyed.
>
> Can we fix this by replacing physindev pointer with plain
> ifindex instead?  There are not too many places that need to
> peek into the original net_device struct, so I don't think
> the additional dev_get_by_index_rcu() would be an issue.

I will work on it, thanks for a good idea!

--
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.