Date:   Fri, 11 May 2018 22:07:43 -0400
From:   "Md. Islam" <mislam4@...t.edu>
To:     "Jason A. Donenfeld" <Jason@...c4.com>
Cc:     Netdev <netdev@...r.kernel.org>
Subject: Re: safe skb resetting after decapsulation and encapsulation

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@...c4.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/

Powered by blists - more mailing lists