netdev - Re: [RFC PATCH 06/17] net: sched: explicit locking in gso

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: <09820d68-ea84-5c9a-bb16-bc99f268c253@gmail.com>
Date:   Wed, 15 Nov 2017 07:11:58 -0800
From:   John Fastabend <john.fastabend@...il.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Eric Dumazet <eric.dumazet@...il.com>, make0818@...il.com,
        Network Development <netdev@...r.kernel.org>,
        Jiří Pírko <jiri@...nulli.us>,
        Cong Wang <xiyou.wangcong@...il.com>
Subject: Re: [RFC PATCH 06/17] net: sched: explicit locking in gso_cpu
 fallback

On 11/14/2017 04:41 PM, Willem de Bruijn wrote:
>>  /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
>>  static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
>>  {
>> -       struct sk_buff *skb = sch->gso_skb;
>> +       struct sk_buff *skb = skb_peek(&sch->gso_skb);
>>
>>         if (skb) {
>> -               sch->gso_skb = NULL;
>> +               skb = __skb_dequeue(&sch->gso_skb);
>>                 qdisc_qstats_backlog_dec(sch, skb);
>>                 sch->q.qlen--;
> 
> In lockless qdiscs, can this race, so that __skb_dequeue returns NULL?
> Same for its use in qdisc_peek_dequeued.
> 

Yes, agree if this was used in lockless qdisc it could race. However,
I don't think it is actually used in the lockless cases yet. For pfifo
fast __skb_array_peek is used.

>> -static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>> +static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
>>  {
> 
> Perhaps dev_requeue_skb_qdisc_locked is more descriptive. Or
> adding a lockdep_is_held(..) also documents that the __locked variant
> below is not just a lock/unlock wrapper around this inner function.
> 

Adding lockdep annotation here makes sense to me.