netdev - Re: Multicast packet loss

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: <20090313.145152.121603300.davem@davemloft.net>
Date:	Fri, 13 Mar 2009 14:51:52 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	dada1@...mosbay.com
Cc:	kchang@...enacr.com, netdev@...r.kernel.org,
	cl@...ux-foundation.org, bmb@...enacr.com
Subject: Re: Multicast packet loss

From: Eric Dumazet <dada1@...mosbay.com>
Date: Mon, 09 Mar 2009 07:36:57 +0100

> I chose cmpxchg() because I needed some form of exclusion here.
> I first added a spinlock inside "struct softirq_del" then I realize
> I could use cmpxchg() instead and keep the structure small. As the
> synchronization is only needed at queueing time, we could pass
> the address of a spinlock XXX to sofirq_del() call.

I don't understand why you need the mutual exclusion in the
first place.  The function pointer always has the same value.
And this locking isn't protecting the list insertion either,
as that isn't even necessary.

It just looks like plain overhead to me.

> Also, when an event was queued for later invocation, I also needed to keep
> a reference on "struct socket" to make sure it doesnt disappear before
> the invocation. Not all sockets are RCU guarded (we added RCU only for 
> some protocols (TCP, UDP ...). So I found keeping a read_lock
> on callback was the easyest thing to do. I now realize we might
> overflow preempt_count, so special care is needed.

You're using this in UDP so... make the rule that you can't use
this with a non-RCU-quiescent protocol.

> About your first point, maybe we should make sofirq_del() (poor name
> I confess) only have one argument (pointer to struct softirq_del),
> and initialize the function pointer at socket init time. That would
> insure "struct softirq_del" is associated to one callback
> only. cmpxchg() test would have to be done on "next" field then (or
> use the spinlock XXX)

Why?  You run this from softirq safe context, nothing can run other
softirqs on this cpu and corrupt the list, therefore.
--
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