netdev - Re: [PATCH net 3/3] net: skb_queue_purge(): lock/unlock the queue only once

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: <20171002075539.113a1eaa@xeon-e3>
Date:   Mon, 2 Oct 2017 07:55:39 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Michael Witten <mfwitten@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Eric Dumazet <eric.dumazet@...il.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 3/3] net: skb_queue_purge(): lock/unlock the queue
 only once

On Mon, 02 Oct 2017 05:15:32 -0000
Michael Witten <mfwitten@...il.com> wrote:

> On Sun, 1 Oct 2017 17:59:09 -0700, Stephen Hemminger wrote:
> 
> > On Sun, 01 Oct 2017 22:19:20 -0000 Michael Witten wrote:
> >  
> >> +	spin_lock_irqsave(&q->lock, flags);
> >> +	skb = q->next;
> >> +	__skb_queue_head_init(q);
> >> +	spin_unlock_irqrestore(&q->lock, flags);  
> >
> > Other code manipulating lists uses splice operation and
> > a sk_buff_head temporary on the stack. That would be easier
> > to understand.
> >
> > 	struct sk_buf_head head;
> >
> > 	__skb_queue_head_init(&head);
> > 	spin_lock_irqsave(&q->lock, flags);
> > 	skb_queue_splice_init(q, &head);
> > 	spin_unlock_irqrestore(&q->lock, flags);
> >
> >  
> >> +	while (skb != head) {
> >> +		next = skb->next;
> >>  		kfree_skb(skb);
> >> +		skb = next;
> >> +	}  
> >
> > It would be cleaner if you could use
> > skb_queue_walk_safe rather than open coding the loop.
> >
> > 	skb_queue_walk_safe(&head, skb,  tmp)
> > 		kfree_skb(skb);  
> 
> I appreciate abstraction as much as anybody, but I do not believe
> that such abstractions would actually be an improvement here.
> 
> * Splice-initing seems more like an idiom than an abstraction;
>   at first blush, it wouldn't be clear to me what the intention
>   is.
> 
> * Such abstractions are fairly unnecessary.
> 
>     * The function as written is already so short as to be
>       easily digested.
> 
>     * More to the point, this function is not some generic,
>       higher-level algorithm that just happens to employ the
>       socket buffer interface; rather, it is a function that
>       implements part of that very interface, and may thus
>       twiddle the intimate bits of these data structures
>       without being accused of abusing a leaky abstraction.
> 
> * Such abstractions add overhead, if only conceptually. In this
>   case, a temporary socket buffer queue allocates *3* unnecessary
>   struct members, including a whole `spinlock_t' member:
>   
>     prev
>     qlen
>     lock
> 
>   It's possible that the compiler will be smart enough to leave
>   those out, but I have my suspicions that it won't, not only
>   given that the interface contract requires that the temporary
>   socket buffer queue be properly initialized before use, but
>   also because splicing into the temporary will manipulate its
>   `qlen'. Yet, why worry whether optimization happens? The whole
>   issue can simply be avoided by exploiting the intimate details
>   that are already philosophically available to us.
> 
>   Similarly, the function `skb_queue_walk_safe' is nice, but it
>   loses value both because a temporary queue loses value (as just
>   described), and because it ignores the fact that legitimate
>   access to the internals of these data structures allows for
>   setting up the requested loop in advance; that is to say, the
>   two parts of the function that we are now debating can be woven
>   together more tightly than `skb_queue_walk_safe' allows.
> 
> For these reasons, I stand by the way that the patch currently
> implements this function; it does exactly what is desired, no more
> or less.
> 
> Sincerely,
> Michael Witten

The point is that there was discussion in the past of replacing
the next/prev as used in skb with more generic code from list.h.
If the abstraction was used, then this code would just work.

The temporary skb_buff_head is on the stack, and any
access to updating those fields like qlen are in CPU cache
and therefore have very little impact on any peformance.