netdev - Re: [PATCH v1] net: WireGuard secure network tunnel

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20191129033205.GA67257@zx2c4.com>
Date:   Fri, 29 Nov 2019 04:32:05 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     David Miller <davem@...emloft.net>
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        gregkh@...uxfoundation.org, torvalds@...ux-foundation.org,
        herbert@...dor.apana.org.au, linux-crypto@...r.kernel.org
Subject: Re: [PATCH v1] net: WireGuard secure network tunnel

Hey Dave,

On Thu, Nov 28, 2019 at 01:30:23PM -0800, David Miller wrote:
> From: "Jason A. Donenfeld" <Jason@...c4.com>
> Date: Wed, 27 Nov 2019 12:26:43 +0100
> 
> > +	do {
> > +		next = skb->next;
> 
> I've been trying desperately to remove all direct references to the SKB list
> implementation details so that we can convert it over to list_head.  This
> means no direct references to skb->next nor skb->prev.
> 
> Please rearrange this using appropriate helpers and abstractions from skbuff.h

I'm not a huge fan of doing manual skb surgery either. The annoying
thing here is that skb_gso_segment returns a list of skbs that's
terminated by the last one's next pointer being NULL. I assume it's this
way so that the GSO code doesn't have to pass a head around. I went to
see what other drivers are doing to deal with the return value of
skb_gso_segment, and found that every place without fail does pretty
much the same thing as me, whether it's wifi drivers, ethernet drivers,
qdiscs, ipsec, etc. Here's (one of) IPsec's usage, for example:

	segs = skb_gso_segment(skb, 0);
	kfree_skb(skb);
	if (IS_ERR(segs))
		return PTR_ERR(segs);
	if (segs == NULL)
		return -EINVAL;

	do {
		struct sk_buff *nskb = segs->next;
		int err;

		skb_mark_not_on_list(segs);
		err = xfrm_output2(net, sk, segs);

		if (unlikely(err)) {
			kfree_skb_list(nskb);
			return err;
		}

		segs = nskb;
	} while (segs);

Given that so much code does the same skb surgery, this seems like it
would be a good opportunity for actually adding the right helper /
abstraction around this. If that sounds good to you, I'll send a commit
adding something like the below, along with fixing up a couple of the
more straight-forward existing places to use the new helper:

#define skb_walk_null_list_safe(first, skb, next)                          \
        for (skb = first, next = skb->next; skb;                           \
             skb = next, next = skb ? skb->next : NULL)

Does this sound good to you? If so, would you like this as lead-up
commits to WireGuard, or just a new independent series all together?

Regards,
Jason