netdev - Re: [PATCH net-next 1/2] udp: Simplify __udp*_lib_mcast

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]

Date:	Tue, 15 Jul 2014 21:53:26 -0400
From:	David Held <drheld@...gle.com>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org, Eric Dumazet <eric.dumazet@...il.com>,
	Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next 1/2] udp: Simplify __udp*_lib_mcast_deliver.

On Tue, Jul 15, 2014 at 8:14 PM, David Miller <davem@...emloft.net> wrote:
> From: David Held <drheld@...gle.com>
> Date: Mon, 14 Jul 2014 18:30:38 -0400
>
>>       spin_lock(&hslot->lock);
>> -     sk = sk_nulls_head(&hslot->head);
>> -     dif = skb->dev->ifindex;
>> -     sk = udp_v4_mcast_next(net, sk, uh->dest, daddr, uh->source, saddr, dif);
>> -     while (sk) {
>> -             stack[count++] = sk;
>> -             sk = udp_v4_mcast_next(net, sk_nulls_next(sk), uh->dest,
>> -                                    daddr, uh->source, saddr, dif);
>> -             if (unlikely(count == ARRAY_SIZE(stack))) {
>> -                     if (!sk)
>> -                             break;
>> -                     flush_stack(stack, count, skb, ~0);
>> -                     count = 0;
>> +     sk_nulls_for_each(sk, node, &hslot->head) {
>> +             if (__udp_is_mcast_sock(net, sk,
>> +                                     uh->dest, daddr,
>> +                                     uh->source, saddr,
>> +                                     dif, hnum)) {
>> +                     stack[count++] = sk;
>> +                     if (unlikely(count == ARRAY_SIZE(stack))) {
>> +                             flush_stack(stack, count, skb, ~0);
>> +                             count = 0;
>> +                     }
>>               }
>>       }
>
> I think you are changing the logic of the loop in the edge case.
>
> If we have exactly ARRAY_SIZE(stack) sockets to process, the old code
> performs the flush_stack() outside of the hslot->lock, but with your
> change we'll do it inside the lock.
>
> The tradeoff here is reducing hslot->lock hold times vs. avoiding
> taking a hold on all the sockets in the stack[] array.
>
> I just wanted to point this out and make sure you are aware of and
> are ok with this.

The followup patch makes it so we always take a hold on the sockets
anyway (it makes things simpler and only changes things for the too
large array case), so might as well reduce the lock time for the
exactly ARRAY_SIZE case.

Should just be a matter of moving the stack addition below the ARRAY_SIZE check:
+                       if (unlikely(count == ARRAY_SIZE(stack))) {
+                               flush_stack(stack, count, skb, ~0);
+                               count = 0;
+                       }
+                       stack[count++] = sk;
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html