safe skb resetting after decapsulation and encapsulation

safe skb resetting after decapsulation and encapsulation

[Jason A. Donenfeld]

Hey Netdev,

A UDP skb comes in via the encap_rcv interface. I do a lot of wild
things to the bytes in the skb -- change where the head starts, modify
a few fragments, decrypt some stuff, trim off some things at the end,
etc. In other words, I'm decapsulating the skb in a pretty intense
way. I benefit from reusing the same skb, performance wise, but after
I'm done processing it, it's really a totally new skb. Eventually it's
time to pass off my skb to netif_receive_skb/netif_rx, but before I do
that, I need to "reinitialize" the skb. (The same goes for when
sending out an skb -- I get it from userspace via ndo_start_xmit, do
crazy things to it, and eventually pass it off to the udp_tunnel send
functions, but first "reinitializing" it.)

At the moment I'm using a function that looks like this:

static void jasons_wild_and_crazy_skb_reset(struct sk_buff *skb)
{
    skb_scrub_packet(skb, true); //1
    memset(&skb->headers_start, 0, offsetof(struct sk_buff,
headers_end) - offsetof(struct sk_buff, headers_start)); //2
    skb->queue_mapping = 0; //3
    skb->nohdr = 0; //4
    skb->peeked = 0; //5
    skb->mac_len = 0; //6
    skb->dev = NULL; //7
#ifdef CONFIG_NET_SCHED
    skb->tc_index = 0; //8
    skb_reset_tc(skb); //9
#endif
    skb->hdr_len = skb_headroom(skb); //10
    skb_reset_mac_header(skb); //11
    skb_reset_network_header(skb); //12
    skb_probe_transport_header(skb, 0); //13
    skb_reset_inner_headers(skb); //14
}

I'm sure that some of this is wrong. Most of it is based on part of an
Octeon ethernet driver I read a few years ago. I numbered each
statement above, hoping to go through it with you all in detail here,
and see what we can cut away and see what we can approve.

1. Obviously correct and required.
2. This is probably wrong. At least it causes crashes when receiving
packets from RHEL 7.5's latest i40e driver in their vendor
frankenkernel, because those flags there have some critical bits
related to allocation. But there are a lot flags in there that I might
consider going through one by one and zeroing out.
3-5. Fields that should be zero, I assume, after
decapsulating/decrypting (and encapsulating/encrypting).
6. WireGuard is layer 3, so there's no mac.
7. We're later going to change the dev this came in on.
8-9: Same flakey rationale as 2,3-5.
10: Since the headroom has changed during the various modifications, I
need to let the packet field know about it.
11-14: The beginning of the headers has changed, and so resetting and
probing is necessary for this to work at all.

So I'm wondering - how much of this is necessary? How much am I
unnecessarily reinventing things that exist elsewhere? I'm pretty sure
in most cases the driver would work with only 1,10-14, but I worry
that bad things would happen in more unusual configurations. I've
tried to systematically go through the entire stack and see where
these might be used or not used, but it seems really inconsistent.

So, I'm writing wondering if somebody has an easy simplification or
rule for handling this kind of intense decapsulation/decryption (and
encapsulation/encryption operation on the other way) operation. I'd
like to make sure I get this down solid.

Thanks,
Jason

Re: safe skb resetting after decapsulation and encapsulation

[Md. Islam]

I'm not an expert on this, but it looks about right. You can take a
look at build_skb() or __build_skb(). It shows the fields that needs
to be set before passing to netif_receive_skb/netif_rx.

On Fri, May 11, 2018 at 6:56 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hey Netdev,
>
> A UDP skb comes in via the encap_rcv interface. I do a lot of wild
> things to the bytes in the skb -- change where the head starts, modify
> a few fragments, decrypt some stuff, trim off some things at the end,
> etc. In other words, I'm decapsulating the skb in a pretty intense
> way. I benefit from reusing the same skb, performance wise, but after
> I'm done processing it, it's really a totally new skb. Eventually it's
> time to pass off my skb to netif_receive_skb/netif_rx, but before I do
> that, I need to "reinitialize" the skb. (The same goes for when
> sending out an skb -- I get it from userspace via ndo_start_xmit, do
> crazy things to it, and eventually pass it off to the udp_tunnel send
> functions, but first "reinitializing" it.)
>
> At the moment I'm using a function that looks like this:
>
> static void jasons_wild_and_crazy_skb_reset(struct sk_buff *skb)
> {
>     skb_scrub_packet(skb, true); //1
>     memset(&skb->headers_start, 0, offsetof(struct sk_buff,
> headers_end) - offsetof(struct sk_buff, headers_start)); //2
>     skb->queue_mapping = 0; //3
>     skb->nohdr = 0; //4
>     skb->peeked = 0; //5
>     skb->mac_len = 0; //6
>     skb->dev = NULL; //7
> #ifdef CONFIG_NET_SCHED
>     skb->tc_index = 0; //8
>     skb_reset_tc(skb); //9
> #endif
>     skb->hdr_len = skb_headroom(skb); //10
>     skb_reset_mac_header(skb); //11
>     skb_reset_network_header(skb); //12
>     skb_probe_transport_header(skb, 0); //13
>     skb_reset_inner_headers(skb); //14
> }
>
> I'm sure that some of this is wrong. Most of it is based on part of an
> Octeon ethernet driver I read a few years ago. I numbered each
> statement above, hoping to go through it with you all in detail here,
> and see what we can cut away and see what we can approve.
>
> 1. Obviously correct and required.
> 2. This is probably wrong. At least it causes crashes when receiving
> packets from RHEL 7.5's latest i40e driver in their vendor
> frankenkernel, because those flags there have some critical bits
> related to allocation. But there are a lot flags in there that I might
> consider going through one by one and zeroing out.
> 3-5. Fields that should be zero, I assume, after
> decapsulating/decrypting (and encapsulating/encrypting).
> 6. WireGuard is layer 3, so there's no mac.
> 7. We're later going to change the dev this came in on.
> 8-9: Same flakey rationale as 2,3-5.
> 10: Since the headroom has changed during the various modifications, I
> need to let the packet field know about it.
> 11-14: The beginning of the headers has changed, and so resetting and
> probing is necessary for this to work at all.
>
> So I'm wondering - how much of this is necessary? How much am I
> unnecessarily reinventing things that exist elsewhere? I'm pretty sure
> in most cases the driver would work with only 1,10-14, but I worry
> that bad things would happen in more unusual configurations. I've
> tried to systematically go through the entire stack and see where
> these might be used or not used, but it seems really inconsistent.
>
> So, I'm writing wondering if somebody has an easy simplification or
> rule for handling this kind of intense decapsulation/decryption (and
> encapsulation/encryption operation on the other way) operation. I'd
> like to make sure I get this down solid.
>
> Thanks,
> Jason



-- 
Tamim
PhD Candidate,
Kent State University
http://web.cs.kent.edu/~mislam4/

Re: safe skb resetting after decapsulation and encapsulation

[Jason A. Donenfeld]

On Sat, May 12, 2018 at 4:07 AM, Md. Islam <mislam4@kent.edu> wrote:
> I'm not an expert on this, but it looks about right.

Really? Even zeroing between headers_start and headers_end? With the
latest RHEL 7.5 kernel's i40e driver, doing this results in a crash in
kfree. It's possible redhat is putting something silly within
header_start and header_end, and so zeroing it is bad, but I suspect
that instead blanket zeroing it like that might actually be incorrect.

> look at build_skb() or __build_skb(). It shows the fields that needs to be set

These just kmalloc a new skb, with most fields set to zero. The ones
it modifies are the ones I'm modifying anyway when messing with the
data the skb contains. Doesn't look like there's much to help there.


I wrote the original post wondering precisely -- which specifically of
1-14 are incorrect, and is there anything specific missing from there.

Re: safe skb resetting after decapsulation and encapsulation

[Md. Islam]

On Sun, May 13, 2018 at 9:24 AM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Sat, May 12, 2018 at 4:07 AM, Md. Islam <mislam4@kent.edu> wrote:
>> I'm not an expert on this, but it looks about right.
>
> Really? Even zeroing between headers_start and headers_end? With the
> latest RHEL 7.5 kernel's i40e driver, doing this results in a crash in
> kfree. It's possible redhat is putting something silly within
> header_start and header_end, and so zeroing it is bad, but I suspect
> that instead blanket zeroing it like that might actually be incorrect.

I had similar issue where I was trying to convert an xdp_buff to
sk_buff. It was crashing in kfree. I figured it was due to skb_shinfo
being empty in my case. skb_skb_shinfo is located at the tail room.
http://vger.kernel.org/~davem/skb_data.html

>
>> look at build_skb() or __build_skb(). It shows the fields that needs to be set
>
> These just kmalloc a new skb, with most fields set to zero. The ones
> it modifies are the ones I'm modifying anyway when messing with the
> data the skb contains. Doesn't look like there's much to help there.
>
>
> I wrote the original post wondering precisely -- which specifically of
> 1-14 are incorrect, and is there anything specific missing from there.