* [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
@ 2012-12-01 17:29 Simon Wunderlich
2012-12-01 18:07 ` [B.A.T.M.A.N.] " Sven Eckelmann
2012-12-01 18:36 ` Eric Dumazet
0 siblings, 2 replies; 9+ messages in thread
From: Simon Wunderlich @ 2012-12-01 17:29 UTC (permalink / raw)
To: davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r, Simon Wunderlich
If rtnl_trylock() is used to prevent circular dependencies, rtnl_unlock()
may destroy this attempt because it first unlocks rtnl_mutex but may
lock it again later. The callgraph roughly looks like:
rtnl_unlock()
netdev_run_todo()
__rtnl_unlock()
netdev_wait_allrefs()
rtnl_lock()
...
__rtnl_unlock()
There are two users which have possible deadlocks and are fixed in this
patch: batman-adv and bridge. Their problematic access pattern is the same:
notifier_call handler:
* holds rtnl lock when called
* calls sysfs code at some point (acquiring sysfs locks)
sysfs code:
* holds sysfs lock when called
* calls rtnl_trylock() and rtnl_unlock(), rtnl_unlock() calls rtnl_lock
implicitly
Fix this by exporting __rtnl_unlock() to just unlock the mutex without
implicitly locking again.
Reported-by: Sven Eckelmann <sven-KaDOiPu9UxWEi8DpZVb4nw@public.gmane.org>
Signed-off-by: Simon Wunderlich <siwu-MaAgPAbsBIVS8oHt8HbXEIQuADTiUCJX@public.gmane.org>
---
Of course, an alternative would be to not lock again after unlocking
within rtnl_unlock(), or put the todo handling into the locked section.
I'm not familiar enough with this code to know what would be best.
There are others using rtnl_trylock(), but I'm not sure if they
are affected.
---
net/batman-adv/sysfs.c | 2 +-
net/bridge/br_sysfs_br.c | 2 +-
net/bridge/br_sysfs_if.c | 2 +-
net/core/rtnetlink.c | 1 +
4 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
index 66518c7..41b74aa 100644
--- a/net/batman-adv/sysfs.c
+++ b/net/batman-adv/sysfs.c
@@ -635,7 +635,7 @@ static ssize_t batadv_store_mesh_iface(struct kobject *kobj,
ret = batadv_hardif_enable_interface(hard_iface, buff);
unlock:
- rtnl_unlock();
+ __rtnl_unlock();
out:
batadv_hardif_free_ref(hard_iface);
return ret;
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index c5c0593..c122782 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
if (!rtnl_trylock())
return restart_syscall();
br_stp_set_enabled(br, val);
- rtnl_unlock();
+ __rtnl_unlock();
return len;
}
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index 13b36bd..d99f394 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -223,7 +223,7 @@ static ssize_t brport_store(struct kobject * kobj,
if (ret == 0)
ret = count;
}
- rtnl_unlock();
+ __rtnl_unlock();
}
return ret;
}
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index fad649a..d95ba6f 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -72,6 +72,7 @@ void __rtnl_unlock(void)
{
mutex_unlock(&rtnl_mutex);
}
+EXPORT_SYMBOL(__rtnl_unlock);
void rtnl_unlock(void)
{
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 17:29 [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock Simon Wunderlich
@ 2012-12-01 18:07 ` Sven Eckelmann
2012-12-01 18:36 ` Eric Dumazet
1 sibling, 0 replies; 9+ messages in thread
From: Sven Eckelmann @ 2012-12-01 18:07 UTC (permalink / raw)
To: b.a.t.m.a.n, bridge, linux-rdma, linux-s390, Andy Gospodarek,
Jay Vosburgh, Divy Le Ray
Cc: Simon Wunderlich, davem, netdev, Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 3266 bytes --]
On Saturday 01 December 2012 18:29:51 Simon Wunderlich wrote:
> If rtnl_trylock() is used to prevent circular dependencies, rtnl_unlock()
> may destroy this attempt because it first unlocks rtnl_mutex but may
> lock it again later. The callgraph roughly looks like:
>
> rtnl_unlock()
> netdev_run_todo()
> __rtnl_unlock()
> netdev_wait_allrefs()
> rtnl_lock()
> ...
> __rtnl_unlock()
>
> There are two users which have possible deadlocks and are fixed in this
> patch: batman-adv and bridge. Their problematic access pattern is the same:
>
> notifier_call handler:
> * holds rtnl lock when called
> * calls sysfs code at some point (acquiring sysfs locks)
>
> sysfs code:
> * holds sysfs lock when called
> * calls rtnl_trylock() and rtnl_unlock(), rtnl_unlock() calls rtnl_lock
> implicitly
>
> Fix this by exporting __rtnl_unlock() to just unlock the mutex without
> implicitly locking again.
>
> Reported-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
>
> ---
> Of course, an alternative would be to not lock again after unlocking
> within rtnl_unlock(), or put the todo handling into the locked section.
> I'm not familiar enough with this code to know what would be best.
>
> There are others using rtnl_trylock(), but I'm not sure if they
> are affected.
At least they look like they have a problem in a parallel user scenario
involving another lock and locking order (most of them s_active or a device
lock). So just to list the places and poke some other users. They can better
decide for themself because they know the code.
drivers/infiniband/ulp/ipoib/ipoib_cm.c: if (!rtnl_trylock())
drivers/infiniband/ulp/ipoib/ipoib_vlan.c: if (!rtnl_trylock())
drivers/infiniband/ulp/ipoib/ipoib_vlan.c: if (!rtnl_trylock())
drivers/net/bonding/bond_alb.c: if (!rtnl_trylock()) {
drivers/net/bonding/bond_main.c: if (!rtnl_trylock()) {
drivers/net/bonding/bond_main.c: if (!rtnl_trylock()) {
drivers/net/bonding/bond_main.c: if (!rtnl_trylock()) {
drivers/net/bonding/bond_main.c: if (!rtnl_trylock()) {
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/bonding/bond_sysfs.c: if (!rtnl_trylock())
drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c: if (!rtnl_trylock()) /*
synchronize with ifdown */
drivers/s390/net/qeth_l2_main.c: if (rtnl_trylock()) {
drivers/s390/net/qeth_l3_main.c: if (rtnl_trylock()) {
net/bridge/br_sysfs_br.c: if (!rtnl_trylock())
net/bridge/br_sysfs_if.c: if (!rtnl_trylock())
net/core/net-sysfs.c: if (!rtnl_trylock())
net/core/net-sysfs.c: if (!rtnl_trylock())
net/core/net-sysfs.c: if (!rtnl_trylock())
net/core/net-sysfs.c: if (!rtnl_trylock())
net/core/net-sysfs.c: if (!rtnl_trylock())
net/ipv4/devinet.c: if (!rtnl_trylock()) {
net/ipv6/addrconf.c: if (!rtnl_trylock())
net/ipv6/addrconf.c: if (!rtnl_trylock())
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 17:29 [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock Simon Wunderlich
2012-12-01 18:07 ` [B.A.T.M.A.N.] " Sven Eckelmann
@ 2012-12-01 18:36 ` Eric Dumazet
2012-12-01 19:04 ` Re: [B.A.T.M.A.N.] " Sven Eckelmann
1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2012-12-01 18:36 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: davem, netdev, b.a.t.m.a.n, Simon Wunderlich
On Sat, 2012-12-01 at 18:29 +0100, Simon Wunderlich wrote:
> If rtnl_trylock() is used to prevent circular dependencies, rtnl_unlock()
> may destroy this attempt because it first unlocks rtnl_mutex but may
> lock it again later. The callgraph roughly looks like:
>
> rtnl_unlock()
> netdev_run_todo()
> __rtnl_unlock()
> netdev_wait_allrefs()
> rtnl_lock()
> ...
> __rtnl_unlock()
>
> There are two users which have possible deadlocks and are fixed in this
> patch: batman-adv and bridge. Their problematic access pattern is the same:
>
> notifier_call handler:
> * holds rtnl lock when called
> * calls sysfs code at some point (acquiring sysfs locks)
>
> sysfs code:
> * holds sysfs lock when called
> * calls rtnl_trylock() and rtnl_unlock(), rtnl_unlock() calls rtnl_lock
> implicitly
>
> Fix this by exporting __rtnl_unlock() to just unlock the mutex without
> implicitly locking again.
>
> Reported-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Simon Wunderlich <siwu@hrz.tu-chemnitz.de>
>
> ---
> Of course, an alternative would be to not lock again after unlocking
> within rtnl_unlock(), or put the todo handling into the locked section.
> I'm not familiar enough with this code to know what would be best.
>
> There are others using rtnl_trylock(), but I'm not sure if they
> are affected.
> ---
> net/batman-adv/sysfs.c | 2 +-
> net/bridge/br_sysfs_br.c | 2 +-
> net/bridge/br_sysfs_if.c | 2 +-
> net/core/rtnetlink.c | 1 +
> 4 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
> index 66518c7..41b74aa 100644
> --- a/net/batman-adv/sysfs.c
> +++ b/net/batman-adv/sysfs.c
> @@ -635,7 +635,7 @@ static ssize_t batadv_store_mesh_iface(struct kobject *kobj,
> ret = batadv_hardif_enable_interface(hard_iface, buff);
>
> unlock:
> - rtnl_unlock();
> + __rtnl_unlock();
> out:
> batadv_hardif_free_ref(hard_iface);
> return ret;
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index c5c0593..c122782 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> if (!rtnl_trylock())
> return restart_syscall();
> br_stp_set_enabled(br, val);
> - rtnl_unlock();
> + __rtnl_unlock();
>
> return len;
> }
I have no idea of why you believe there is a problem here.
Could you explain how net_todo_list could be not empty ?
As long as no device is unregistered between
rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 18:36 ` Eric Dumazet
@ 2012-12-01 19:04 ` Sven Eckelmann
2012-12-01 19:44 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Sven Eckelmann @ 2012-12-01 19:04 UTC (permalink / raw)
To: b.a.t.m.a.n
Cc: Eric Dumazet, Simon Wunderlich, netdev, davem, Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]
On Saturday 01 December 2012 10:36:12 Eric Dumazet wrote:
[...]
> > diff --git a/net/batman-adv/sysfs.c b/net/batman-adv/sysfs.c
> > index 66518c7..41b74aa 100644
> > --- a/net/batman-adv/sysfs.c
> > +++ b/net/batman-adv/sysfs.c
> > @@ -635,7 +635,7 @@ static ssize_t batadv_store_mesh_iface(struct kobject
> > *kobj,>
> > ret = batadv_hardif_enable_interface(hard_iface, buff);
> >
> > unlock:
> > - rtnl_unlock();
> > + __rtnl_unlock();
> >
> > out:
> > batadv_hardif_free_ref(hard_iface);
> > return ret;
> >
> > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> > index c5c0593..c122782 100644
> > --- a/net/bridge/br_sysfs_br.c
> > +++ b/net/bridge/br_sysfs_br.c
> > @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> >
> > if (!rtnl_trylock())
> >
> > return restart_syscall();
> >
> > br_stp_set_enabled(br, val);
> >
> > - rtnl_unlock();
> > + __rtnl_unlock();
> >
> > return len;
> >
> > }
>
> I have no idea of why you believe there is a problem here.
>
> Could you explain how net_todo_list could be not empty ?
>
> As long as no device is unregistered between
> rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
I am not sure what "here" means for your. At least batman-adv tries to
unregister a device -> problem [1]. I will not make any judgements about the
other uses in the kernel/other parts patched by Simon.
Kind regards,
Sven
[1] http://article.gmane.org/gmane.linux.kernel/1392295
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 19:04 ` Re: [B.A.T.M.A.N.] " Sven Eckelmann
@ 2012-12-01 19:44 ` Eric Dumazet
2012-12-01 19:57 ` Sven Eckelmann
2012-12-01 20:01 ` Simon Wunderlich
0 siblings, 2 replies; 9+ messages in thread
From: Eric Dumazet @ 2012-12-01 19:44 UTC (permalink / raw)
To: Sven Eckelmann
Cc: b.a.t.m.a.n, Simon Wunderlich, netdev, davem, Simon Wunderlich
On Sat, 2012-12-01 at 20:04 +0100, Sven Eckelmann wrote:
> On Saturday 01 December 2012 10:36:12 Eric Dumazet wrote:
>
> > > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> > > index c5c0593..c122782 100644
> > > --- a/net/bridge/br_sysfs_br.c
> > > +++ b/net/bridge/br_sysfs_br.c
> > > @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> > >
> > > if (!rtnl_trylock())
> > >
> > > return restart_syscall();
> > >
> > > br_stp_set_enabled(br, val);
> > >
> > > - rtnl_unlock();
> > > + __rtnl_unlock();
> > >
> > > return len;
> > >
> > > }
> >
> > I have no idea of why you believe there is a problem here.
> >
> > Could you explain how net_todo_list could be not empty ?
> >
> > As long as no device is unregistered between
> > rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
>
> I am not sure what "here" means for your. At least batman-adv tries to
> unregister a device -> problem [1]. I will not make any judgements about the
> other uses in the kernel/other parts patched by Simon.
>
I was reacting to the change in net/bridge/br_sysfs_br.c
rtnl_trylock() could set a boolean flag to explicitly WARN_ON()
in case we try to unregister a device.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 19:44 ` Eric Dumazet
@ 2012-12-01 19:57 ` Sven Eckelmann
2012-12-01 20:01 ` Simon Wunderlich
1 sibling, 0 replies; 9+ messages in thread
From: Sven Eckelmann @ 2012-12-01 19:57 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r,
davem-fT/PcQaiUtIeIZ0/mPfg9Q, Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 798 bytes --]
On Saturday 01 December 2012 11:44:34 Eric Dumazet wrote:
[...]
> > > I have no idea of why you believe there is a problem here.
> > >
> > > Could you explain how net_todo_list could be not empty ?
> > >
> > > As long as no device is unregistered between
> > > rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
> >
> > I am not sure what "here" means for your. At least batman-adv tries to
> > unregister a device -> problem [1]. I will not make any judgements about
> > the other uses in the kernel/other parts patched by Simon.
>
> I was reacting to the change in net/bridge/br_sysfs_br.c
Yes, in this context it makes more sense.
> rtnl_trylock() could set a boolean flag to explicitly WARN_ON()
> in case we try to unregister a device.
Sounds interesting.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 19:44 ` Eric Dumazet
2012-12-01 19:57 ` Sven Eckelmann
@ 2012-12-01 20:01 ` Simon Wunderlich
2012-12-03 20:09 ` Re: [B.A.T.M.A.N.] " Antonio Quartulli
1 sibling, 1 reply; 9+ messages in thread
From: Simon Wunderlich @ 2012-12-01 20:01 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
b.a.t.m.a.n-ZwoEplunGu2X36UT3dwllkB+6BGkLq7r, Simon Wunderlich,
davem-fT/PcQaiUtIeIZ0/mPfg9Q
[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]
On Sat, Dec 01, 2012 at 11:44:34AM -0800, Eric Dumazet wrote:
> On Sat, 2012-12-01 at 20:04 +0100, Sven Eckelmann wrote:
> > On Saturday 01 December 2012 10:36:12 Eric Dumazet wrote:
> >
> > > > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> > > > index c5c0593..c122782 100644
> > > > --- a/net/bridge/br_sysfs_br.c
> > > > +++ b/net/bridge/br_sysfs_br.c
> > > > @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> > > >
> > > > if (!rtnl_trylock())
> > > >
> > > > return restart_syscall();
> > > >
> > > > br_stp_set_enabled(br, val);
> > > >
> > > > - rtnl_unlock();
> > > > + __rtnl_unlock();
> > > >
> > > > return len;
> > > >
> > > > }
> > >
> > > I have no idea of why you believe there is a problem here.
> > >
> > > Could you explain how net_todo_list could be not empty ?
> > >
> > > As long as no device is unregistered between
> > > rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
> >
> > I am not sure what "here" means for your. At least batman-adv tries to
> > unregister a device -> problem [1]. I will not make any judgements about the
> > other uses in the kernel/other parts patched by Simon.
> >
>
> I was reacting to the change in net/bridge/br_sysfs_br.c
>
> rtnl_trylock() could set a boolean flag to explicitly WARN_ON()
> in case we try to unregister a device.
Well, I'm not sure if this can happen in the bridge code, but from looking at the
code it doesn't appear to be impossible. It would be better to be sure that it can't
deadlock IMHO.
(Although doing unlock/lock/unlock in an unlock function is also a little "uncommon").
Cheers,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-01 20:01 ` Simon Wunderlich
@ 2012-12-03 20:09 ` Antonio Quartulli
2012-12-03 20:50 ` Sven Eckelmann
0 siblings, 1 reply; 9+ messages in thread
From: Antonio Quartulli @ 2012-12-03 20:09 UTC (permalink / raw)
To: Eric Dumazet
Cc: Simon Wunderlich, Sven Eckelmann, b.a.t.m.a.n, netdev, davem,
Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]
On Sat, Dec 01, 2012 at 09:01:53PM +0100, Simon Wunderlich wrote:
> On Sat, Dec 01, 2012 at 11:44:34AM -0800, Eric Dumazet wrote:
> > On Sat, 2012-12-01 at 20:04 +0100, Sven Eckelmann wrote:
> > > On Saturday 01 December 2012 10:36:12 Eric Dumazet wrote:
> > >
> > > > > diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> > > > > index c5c0593..c122782 100644
> > > > > --- a/net/bridge/br_sysfs_br.c
> > > > > +++ b/net/bridge/br_sysfs_br.c
> > > > > @@ -142,7 +142,7 @@ static ssize_t store_stp_state(struct device *d,
> > > > >
> > > > > if (!rtnl_trylock())
> > > > >
> > > > > return restart_syscall();
> > > > >
> > > > > br_stp_set_enabled(br, val);
> > > > >
> > > > > - rtnl_unlock();
> > > > > + __rtnl_unlock();
> > > > >
> > > > > return len;
> > > > >
> > > > > }
> > > >
> > > > I have no idea of why you believe there is a problem here.
> > > >
> > > > Could you explain how net_todo_list could be not empty ?
> > > >
> > > > As long as no device is unregistered between
> > > > rtnl_trylock()/rtnl_unlock(), there is no possible deadlock.
> > >
> > > I am not sure what "here" means for your. At least batman-adv tries to
> > > unregister a device -> problem [1]. I will not make any judgements about the
> > > other uses in the kernel/other parts patched by Simon.
> > >
> >
> > I was reacting to the change in net/bridge/br_sysfs_br.c
> >
> > rtnl_trylock() could set a boolean flag to explicitly WARN_ON()
> > in case we try to unregister a device.
>
> Well, I'm not sure if this can happen in the bridge code, but from looking at the
> code it doesn't appear to be impossible. It would be better to be sure that it can't
> deadlock IMHO.
>
> (Although doing unlock/lock/unlock in an unlock function is also a little "uncommon").
But still we have the problem in batman-adv (as Sven pointed out in a previous
email) that tries to unregister a device in that "critical window".
Exporting __rtnl_unlock() would solve the issue in this case.
If you think the bridge code should not end up in such situation, what if Simon
resends the patch with only the __rtnl_unlock() exportation and the change in
batman-adv?
Cheers,
--
Antonio Quartulli
..each of us alone is worth nothing..
Ernesto "Che" Guevara
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Re: Re: [B.A.T.M.A.N.] [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock
2012-12-03 20:09 ` Re: [B.A.T.M.A.N.] " Antonio Quartulli
@ 2012-12-03 20:50 ` Sven Eckelmann
0 siblings, 0 replies; 9+ messages in thread
From: Sven Eckelmann @ 2012-12-03 20:50 UTC (permalink / raw)
To: Antonio Quartulli
Cc: Eric Dumazet, Simon Wunderlich, b.a.t.m.a.n, netdev, davem,
Simon Wunderlich
[-- Attachment #1: Type: text/plain, Size: 954 bytes --]
On Monday 03 December 2012 21:09:06 Antonio Quartulli wrote:
> But still we have the problem in batman-adv (as Sven pointed out in a
> previous email) that tries to unregister a device in that "critical
> window".
>
> Exporting __rtnl_unlock() would solve the issue in this case.
>
> If you think the bridge code should not end up in such situation, what if
> Simon resends the patch with only the __rtnl_unlock() exportation and the
> change in batman-adv?
I personally have big doubts about the removal of the second half of the
unregister. Doesn't sound like the best idea. This would result in side
effects... one of them for example would be the possible deadlock scenario
moved to the other users of rtnl_trylock which don't unregister a device
inside their critical section.... so either you do it always this way when
using rtnl_trylock or it will break. I don't want to imagine other problems
caused by this change.
Kind regards,
Sven
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-12-03 20:50 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-01 17:29 [PATCH] net: fix possible deadlocks in rtnl_trylock/unlock Simon Wunderlich
2012-12-01 18:07 ` [B.A.T.M.A.N.] " Sven Eckelmann
2012-12-01 18:36 ` Eric Dumazet
2012-12-01 19:04 ` Re: [B.A.T.M.A.N.] " Sven Eckelmann
2012-12-01 19:44 ` Eric Dumazet
2012-12-01 19:57 ` Sven Eckelmann
2012-12-01 20:01 ` Simon Wunderlich
2012-12-03 20:09 ` Re: [B.A.T.M.A.N.] " Antonio Quartulli
2012-12-03 20:50 ` Sven Eckelmann
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).