Hey Dave,
On Thu, Nov 28, 2019 at 01:30:23PM -0800, David Miller wrote:
> From: "Jason A. Donenfeld" <Jason@xxxxxxxxx>
> 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
