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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANP3RGc=U4g7aGfX9Hmi24FGQ0daBXLVv_S=Srk288x57amVDg@mail.gmail.com>
Date: Thu, 5 Jun 2025 15:50:31 +0200
From: Maciej Żenczykowski <maze@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, andrew+netdev@...n.ch, horms@...nel.org, 
	martin.lau@...ux.dev, daniel@...earbox.net, john.fastabend@...il.com, 
	eddyz87@...il.com, sdf@...ichev.me, haoluo@...gle.com, willemb@...gle.com, 
	william.xuanziyang@...wei.com, alan.maguire@...cle.com, bpf@...r.kernel.org
Subject: Re: [PATCH net] net: clear the dst when changing skb protocol

On Thu, Jun 5, 2025 at 3:22 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> 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?

It does make a fair bit of sense.
Question: does calling it as a kfunc require kernel BTF?
Specifically some ram limited devices want to disable CONFIG_DEBUG_INFO_BTF...
I know normal bpf helpers don't need that...
I guess you could always convert ipv4 -> ipv6 -> ipv4 ;-)
--
Maciej Żenczykowski, Kernel Networking Developer @ Google