netdev - Re: [PATCH bpf-next 0/3] bpf: introduce skb refcount kfuncs

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: <Z73TS/tjk9okSqlC@boxer>
Date: Tue, 25 Feb 2025 15:27:23 +0100
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
CC: bpf <bpf@...r.kernel.org>, Alexei Starovoitov <ast@...nel.org>, "Daniel
 Borkmann" <daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>,
	"Network Development" <netdev@...r.kernel.org>, "Karlsson, Magnus"
	<magnus.karlsson@...el.com>, Martin KaFai Lau <martin.lau@...ux.dev>
Subject: Re: [PATCH bpf-next 0/3] bpf: introduce skb refcount kfuncs

On Fri, Feb 21, 2025 at 05:55:57PM -0800, Alexei Starovoitov wrote:
> On Thu, Feb 20, 2025 at 5:45 AM Maciej Fijalkowski
> <maciej.fijalkowski@...el.com> wrote:
> >
> > Hi!
> >
> > This patchset provides what is needed for storing skbs as kptrs in bpf
> > maps. We start with necessary kernel change as discussed at [0] with
> > Martin, then next patch adds kfuncs for handling skb refcount and on top
> > of that a test case is added where one program stores skbs and then next
> > program is able to retrieve them from map.
> >
> > Martin, regarding the kernel change I decided to go with boolean
> > approach instead of what you initially suggested. Let me know if it
> > works for you.
> >
> > Thanks,
> > Maciej
> >
> > [0]: https://lore.kernel.org/bpf/Z0X%2F9PhIhvQwsgfW@boxer/
> 
> Before we go further we need a lot more details on "why" part.
> In the old thread I was able to find:
> 
> > On TC egress hook skb is stored in a map ...
> > During TC ingress hook on the same interface, the skb that was previously
> stored in map is retrieved ...
> 
> This is too cryptic. I see several concerns with such use case
> including netns crossing, L2/L3 mismatch, skb_scrub.
> 
> I doubt we can make such "skb stash in a map" safe without
> restricting the usage, so please provide detailed
> description of the use case.

Hi Alexei,

We have a system with two nodes: one is a fully fledged Linux system (big node)
and the other one a smaller embedded system. The big node runs Linux PTP for
time synchronization, the smaller node we have no control over (might run Linux
or something else). The problem is that we would like to use the Tx timestamps
from the small node in the Linux PTP application on the big node. When a packet
is sent out from the big node it arrives at the small node that send it out one
of its interfaces. It then replies with another packet back to the big node
with the Tx timestamp in it.

Our current PoC for attacking this is to store the skb in a map (using this
patch set) when it is sent out from the big node then retrieve it from the map
when the reply from the small node is received. We then take the timestamp from
the packet and put it in the skb and send it up to the socket error queue so
that Linux PTP works out of the box.

Note that for the purpose of setting skb->hwtstamp and sending it up to the
error queue we are adding a kfunc (bpf_tx_tstamp) responsible for it, which is
not included in this set, so I understand it is not obvious what we achieved
with the current form of this patch set being discussed.

We did not consider the restrictions that should be implemented from netns POV,
so that is a good point. How would you see this being fixed?