Re: [PATCH net] net: clear the dst when changing skb protocol

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 4 Jun 2025 23:21:02 +0200 Maciej Żenczykowski wrote:
> > @@ -3550,10 +3557,10 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff,
> >                 /* Match skb->protocol to new outer l3 protocol */
> >                 if (skb->protocol == htons(ETH_P_IP) &&
> >                     flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV6)
> > -                       skb->protocol = htons(ETH_P_IPV6);
> > +                       bpf_skb_change_protocol(skb, ETH_P_IPV6);
> >                 else if (skb->protocol == htons(ETH_P_IPV6) &&
> >                          flags & BPF_F_ADJ_ROOM_ENCAP_L3_IPV4)
> > -                       skb->protocol = htons(ETH_P_IP);
> > +                       bpf_skb_change_protocol(skb, ETH_P_IP);  
> 
> I wonder if this shouldn't drop dst even when doing ipv4->ipv4 or
> ipv6->ipv6 -- it's encapping, presumably old dst is irrelevant...

I keep going back and forth on this. You definitely have a point, 
but I feel like there are levels to how BPF prog can make the dst
irrelevant:
 - change proto
 - encap
 - adjust room but not set any encap flag
 - overwrite the addrs without calling any helpers
First case we have to cover for safety, last we can't possibly cover.
So the question is whether we should draw the line somewhere in
the middle, or leave this patch as is and if the actual use case arrives
- let BPF call skb_dst_drop() as a kfunc. Right now I'm leaning towards
the latter.

Does that make sense? Does anyone else have an opinion?