Re: [PATCH nf] netfilter: nf_queue: be more careful with sk refcounts

[Florian Westphal <fw@strlen.de>]

Joe Stringer <joe@cilium.io> wrote:
> On Mon, Feb 28, 2022 at 8:29 AM Florian Westphal <fw@strlen.de> wrote:
> >
> > Eric Dumazet says:
> >   The sock_hold() side seems suspect, because there is no guarantee
> >   that sk_refcnt is not already 0.
> >
> > Also, there is a way to wire up skb->sk in a way that skb doesn't hold
> > a reference on the socket, so we need to check for that as well.
> >
> > For refcount-less skb->sk case, try to increment the reference count
> > and then override the destructor.
> >
> > On failure, we cannot queue the packet and need to indicate an
> > error.  THe packet will be dropped by the caller.
> >
> > Cc: Joe Stringer <joe@cilium.io>
> > Fixes: 271b72c7fa82c ("udp: RCU handling for Unicast packets.")
> 
> Hi Florian, thanks for the fix.
> 
> skb_sk_is_prefetched() was introduced in commit cf7fbe660f2d ("bpf:
> Add socket assign support"). You may want to split the hunk below into
> a dedicated patch for this reason.

Yes, I see, that helps with backports, will do.

> > +       if (skb_sk_is_prefetched(skb)) {
> > +               struct sock *sk = skb->sk;
> > +
> > +               if (!sk_is_refcounted(sk)) {
> > +                       if (!refcount_inc_not_zero(&sk->sk_refcnt))
> > +                               return -ENOTCONN;
> > +
> > +                       /* drop refcount on skb_orphan */
> > +                       skb->destructor = sock_edemux;
> > +               }
> > +       }
> > +
> >         entry = kmalloc(sizeof(*entry) + route_key_size, GFP_ATOMIC);
> >         if (!entry)
> >                 return -ENOMEM;
> 
> I've never heard of someone trying to use socket prefetch /
> bpf_sk_assign in conjunction with nf_queue, bit of an unusual case.

Me neither, but if someone does it, skb->sk leaves rcu protection.

> Given that `skb_sk_is_prefetched()` relies on the skb->destructor
> pointing towards sock_pfree, and this code would change that to
> sock_edemux, the difference the patch would make is this: if the
> packet is queued and then accepted, the socket prefetch selection
> could be ignored.

Hmmm, wait a second, is that because of orphan in input path, i.e.,
that this preselect has to work even across veth/netns crossing?

> I looked closely at this hunk, I didn't look closely at the rest of
> the patch. Assuming you split just this hunk into a dedicated patch,
> you can add my Ack:
> 
> Acked-by: Joe Stringer <joe@cilium.io>

Thats what I'll do, thanks Joe!