From: Pravin B Shelar <hidden> Date: 2013-08-22 03:58:10
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
CC: Jesse Gross <redacted>
CC: Johannes Berg <redacted>
Signed-off-by: Pravin B Shelar <redacted>
---
net/netlink/genetlink.c | 46 +++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 41 insertions(+), 5 deletions(-)
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-22 07:36:25
On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
As I've commented over in the other thread, I really think this is the
wrong thing to do. It was never the case that dumpit() was actually
locked under genl_lock(), and adding it doesn't really help anyone.
johannes
On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
[off-list ref] wrote:
On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
quoted
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
As I've commented over in the other thread, I really think this is the
wrong thing to do. It was never the case that dumpit() was actually
locked under genl_lock(), and adding it doesn't really help anyone.
If you look at commit def3117493eafd9df , it removes assignment of
cb_mutex to genl_mutex. Thats how genl-dump operation was called under
genl_mutex.
This patch simply restores that locking.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-22 17:51:09
On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
[off-list ref] wrote:
quoted
On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
quoted
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
As I've commented over in the other thread, I really think this is the
wrong thing to do. It was never the case that dumpit() was actually
locked under genl_lock(), and adding it doesn't really help anyone.
If you look at commit def3117493eafd9df , it removes assignment of
cb_mutex to genl_mutex. Thats how genl-dump operation was called under
genl_mutex.
This patch simply restores that locking.
Huh, ok, so you actually caused all the dump locking bugs in
nl80211! :-)
johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-22 17:51:50
On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
[off-list ref] wrote:
quoted
On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
quoted
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
As I've commented over in the other thread, I really think this is the
wrong thing to do. It was never the case that dumpit() was actually
locked under genl_lock(), and adding it doesn't really help anyone.
If you look at commit def3117493eafd9df , it removes assignment of
cb_mutex to genl_mutex. Thats how genl-dump operation was called under
genl_mutex.
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
johannes
On Thu, Aug 22, 2013 at 10:51 AM, Johannes Berg
[off-list ref] wrote:
On Thu, 2013-08-22 at 10:42 -0700, Pravin Shelar wrote:
quoted
On Thu, Aug 22, 2013 at 12:36 AM, Johannes Berg
[off-list ref] wrote:
quoted
On Wed, 2013-08-21 at 20:58 -0700, Pravin B Shelar wrote:
quoted
In case of genl-family with parallel ops off, dumpif() callback
is expected to run under genl_lock, But commit def3117493eafd9df
(genl: Allow concurrent genl callbacks.) changed this behaviour
where only first dumpit() op was called under genl-lock.
For subsequent dump, only nlk->cb_lock was taken.
Following patch fixes it by defining locked dumpit() and done()
callback which takes care of genl-locking.
As I've commented over in the other thread, I really think this is the
wrong thing to do. It was never the case that dumpit() was actually
locked under genl_lock(), and adding it doesn't really help anyone.
If you look at commit def3117493eafd9df , it removes assignment of
cb_mutex to genl_mutex. Thats how genl-dump operation was called under
genl_mutex.
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
This replaces global genl-lock with per-socket lock which allows
parallel operation in parallel-genl-families and they are not blocked
due to other unrelated genl-family operations.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-22 18:19:03
On Thu, 2013-08-22 at 11:10 -0700, Pravin Shelar wrote:
quoted
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
This replaces global genl-lock with per-socket lock which allows
parallel operation in parallel-genl-families and they are not blocked
due to other unrelated genl-family operations.
I don't think so? It replaces the genl lock as cb_mutex with a per
*kernel* socket lock, so really just one per network namespace. That's
not much of an improvement really.
johannes
On Thu, Aug 22, 2013 at 11:18 AM, Johannes Berg
[off-list ref] wrote:
On Thu, 2013-08-22 at 11:10 -0700, Pravin Shelar wrote:
quoted
quoted
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
This replaces global genl-lock with per-socket lock which allows
parallel operation in parallel-genl-families and they are not blocked
due to other unrelated genl-family operations.
I don't think so? It replaces the genl lock as cb_mutex with a per
*kernel* socket lock, so really just one per network namespace. That's
not much of an improvement really.
You are thinking abt genl-sock -> nlk -> cb_lock which is not used in
genl-dump locking if cb_lock is NULL while creating nl socket.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-23 07:29:44
On Thu, 2013-08-22 at 13:31 -0700, Pravin Shelar wrote:
On Thu, Aug 22, 2013 at 11:18 AM, Johannes Berg
[off-list ref] wrote:
quoted
On Thu, 2013-08-22 at 11:10 -0700, Pravin Shelar wrote:
quoted
quoted
By the way - why? This just means that netlink will allocate another
lock to lock it all, so it's not a very useful change?
This replaces global genl-lock with per-socket lock which allows
parallel operation in parallel-genl-families and they are not blocked
due to other unrelated genl-family operations.
I don't think so? It replaces the genl lock as cb_mutex with a per
*kernel* socket lock, so really just one per network namespace. That's
not much of an improvement really.
You are thinking abt genl-sock -> nlk -> cb_lock which is not used in
genl-dump locking if cb_lock is NULL while creating nl socket.
You're right, this is also done for each userland socket, so each socket
now gets its own cb_mutex. This would help genl families with
parallel_ops.
johannes
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-23 09:51:29
On Fri, 2013-08-23 at 09:29 +0200, Johannes Berg wrote:
quoted
You are thinking abt genl-sock -> nlk -> cb_lock which is not used in
genl-dump locking if cb_lock is NULL while creating nl socket.
You're right, this is also done for each userland socket, so each socket
now gets its own cb_mutex. This would help genl families with
parallel_ops.
I'm still missing something. Kernel 3.4 had cb_mutex assign to the
genl_mutex, but we saw the original crash there, apparently dumpit
*wasn't* (always) locked with it?
johannes
On Fri, Aug 23, 2013 at 2:51 AM, Johannes Berg
[off-list ref] wrote:
On Fri, 2013-08-23 at 09:29 +0200, Johannes Berg wrote:
quoted
quoted
You are thinking abt genl-sock -> nlk -> cb_lock which is not used in
genl-dump locking if cb_lock is NULL while creating nl socket.
You're right, this is also done for each userland socket, so each socket
now gets its own cb_mutex. This would help genl families with
parallel_ops.
I'm still missing something. Kernel 3.4 had cb_mutex assign to the
genl_mutex, but we saw the original crash there, apparently dumpit
*wasn't* (always) locked with it?
Can you point me to original crash on 3.4?
Thanks.
From: Johannes Berg <johannes@sipsolutions.net> Date: 2013-08-26 06:06:47
On Fri, 2013-08-23 at 13:52 -0700, Pravin Shelar wrote:
quoted
I'm still missing something. Kernel 3.4 had cb_mutex assign to the
genl_mutex, but we saw the original crash there, apparently dumpit
*wasn't* (always) locked with it?