* [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-28 15:44 ` Petr Machata
0 siblings, 0 replies; 14+ messages in thread
From: Petr Machata @ 2018-05-28 15:44 UTC (permalink / raw)
To: bridge, netdev; +Cc: stephen, davem
Callers of br_fdb_find() need to hold the hash lock, which
br_fdb_find_port() doesn't do. Add the missing lock/unlock
pair.
Signed-off-by: Petr Machata <petrm@mellanox.com>
---
net/bridge/br_fdb.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
index b19e310..3f5691a 100644
--- a/net/bridge/br_fdb.c
+++ b/net/bridge/br_fdb.c
@@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
return NULL;
br = netdev_priv(br_dev);
+ spin_lock_bh(&br->hash_lock);
f = br_fdb_find(br, addr, vid);
if (f && f->dst)
dev = f->dst->dev;
+ spin_unlock_bh(&br->hash_lock);
return dev;
}
--
2.4.11
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-28 15:44 ` Petr Machata
@ 2018-05-28 15:52 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2018-05-28 15:52 UTC (permalink / raw)
To: Petr Machata, bridge, netdev; +Cc: davem
On 28/05/18 18:44, Petr Machata wrote:
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
> net/bridge/br_fdb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b19e310..3f5691a 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
> return NULL;
>
> br = netdev_priv(br_dev);
> + spin_lock_bh(&br->hash_lock);
> f = br_fdb_find(br, addr, vid);
> if (f && f->dst)
> dev = f->dst->dev;
> + spin_unlock_bh(&br->hash_lock);
>
> return dev;
> }
>
There's also a lockdep assert for hash_lock in br_find_fdb().
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-28 15:52 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2018-05-28 15:52 UTC (permalink / raw)
To: Petr Machata, bridge, netdev; +Cc: davem
On 28/05/18 18:44, Petr Machata wrote:
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
> net/bridge/br_fdb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b19e310..3f5691a 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
> return NULL;
>
> br = netdev_priv(br_dev);
> + spin_lock_bh(&br->hash_lock);
> f = br_fdb_find(br, addr, vid);
> if (f && f->dst)
> dev = f->dst->dev;
> + spin_unlock_bh(&br->hash_lock);
>
> return dev;
> }
>
There's also a lockdep assert for hash_lock in br_find_fdb().
Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-28 15:52 ` Nikolay Aleksandrov
@ 2018-05-28 15:53 ` Nikolay Aleksandrov
-1 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2018-05-28 15:53 UTC (permalink / raw)
To: Petr Machata, bridge, netdev; +Cc: davem
On 28/05/18 18:52, Nikolay Aleksandrov wrote:
> On 28/05/18 18:44, Petr Machata wrote:
>> Callers of br_fdb_find() need to hold the hash lock, which
>> br_fdb_find_port() doesn't do. Add the missing lock/unlock
>> pair.
>>
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>> ---
>> net/bridge/br_fdb.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index b19e310..3f5691a 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
>> return NULL;
>>
>> br = netdev_priv(br_dev);
>> + spin_lock_bh(&br->hash_lock);
>> f = br_fdb_find(br, addr, vid);
>> if (f && f->dst)
>> dev = f->dst->dev;
>> + spin_unlock_bh(&br->hash_lock);
>>
>> return dev;
>> }
>>
>
> There's also a lockdep assert for hash_lock in br_find_fdb().
>
> Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
Fixes: 4d4fd36126d6 ("net: bridge: Publish bridge accessor functions")
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-28 15:53 ` Nikolay Aleksandrov
0 siblings, 0 replies; 14+ messages in thread
From: Nikolay Aleksandrov @ 2018-05-28 15:53 UTC (permalink / raw)
To: Petr Machata, bridge, netdev; +Cc: davem
On 28/05/18 18:52, Nikolay Aleksandrov wrote:
> On 28/05/18 18:44, Petr Machata wrote:
>> Callers of br_fdb_find() need to hold the hash lock, which
>> br_fdb_find_port() doesn't do. Add the missing lock/unlock
>> pair.
>>
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>> ---
>> net/bridge/br_fdb.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
>> index b19e310..3f5691a 100644
>> --- a/net/bridge/br_fdb.c
>> +++ b/net/bridge/br_fdb.c
>> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
>> return NULL;
>>
>> br = netdev_priv(br_dev);
>> + spin_lock_bh(&br->hash_lock);
>> f = br_fdb_find(br, addr, vid);
>> if (f && f->dst)
>> dev = f->dst->dev;
>> + spin_unlock_bh(&br->hash_lock);
>>
>> return dev;
>> }
>>
>
> There's also a lockdep assert for hash_lock in br_find_fdb().
>
> Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
>
Fixes: 4d4fd36126d6 ("net: bridge: Publish bridge accessor functions")
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-28 15:53 ` Nikolay Aleksandrov
@ 2018-05-28 16:19 ` Petr Machata
-1 siblings, 0 replies; 14+ messages in thread
From: Petr Machata @ 2018-05-28 16:19 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: netdev, bridge, davem
Nikolay Aleksandrov <nikolay@cumulusnetworks.com> writes:
> Fixes: 4d4fd36126d6 ("net: bridge: Publish bridge accessor functions")
Correct.
Thanks,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-28 15:44 ` Petr Machata
@ 2018-05-28 17:42 ` Stephen Hemminger
-1 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2018-05-28 17:42 UTC (permalink / raw)
To: Petr Machata; +Cc: netdev, bridge, davem
On Mon, 28 May 2018 17:44:16 +0200
Petr Machata <petrm@mellanox.com> wrote:
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
> net/bridge/br_fdb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b19e310..3f5691a 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
> return NULL;
>
> br = netdev_priv(br_dev);
> + spin_lock_bh(&br->hash_lock);
> f = br_fdb_find(br, addr, vid);
> if (f && f->dst)
> dev = f->dst->dev;
> + spin_unlock_bh(&br->hash_lock);
>
> return dev;
> }
Sigh. when did br_fdb_find start needing hash_lock?
What is the point of RCU then?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-28 17:42 ` Stephen Hemminger
0 siblings, 0 replies; 14+ messages in thread
From: Stephen Hemminger @ 2018-05-28 17:42 UTC (permalink / raw)
To: Petr Machata; +Cc: bridge, netdev, davem
On Mon, 28 May 2018 17:44:16 +0200
Petr Machata <petrm@mellanox.com> wrote:
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> ---
> net/bridge/br_fdb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/bridge/br_fdb.c b/net/bridge/br_fdb.c
> index b19e310..3f5691a 100644
> --- a/net/bridge/br_fdb.c
> +++ b/net/bridge/br_fdb.c
> @@ -135,9 +135,11 @@ struct net_device *br_fdb_find_port(const struct net_device *br_dev,
> return NULL;
>
> br = netdev_priv(br_dev);
> + spin_lock_bh(&br->hash_lock);
> f = br_fdb_find(br, addr, vid);
> if (f && f->dst)
> dev = f->dst->dev;
> + spin_unlock_bh(&br->hash_lock);
>
> return dev;
> }
Sigh. when did br_fdb_find start needing hash_lock?
What is the point of RCU then?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-28 15:44 ` Petr Machata
@ 2018-05-30 16:42 ` David Miller
-1 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-05-30 16:42 UTC (permalink / raw)
To: petrm; +Cc: netdev, bridge
From: Petr Machata <petrm@mellanox.com>
Date: Mon, 28 May 2018 17:44:16 +0200
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
If all of the these uses of br_fdb_find_port() are safe, then it
should use the RCU fdb lookup variant.
So I basically agree with Stephen that this locking doesn't make any
sense.
The lock is needed when you are going to add or delete an FDB entry.
Here we are doing a lookup and returning a device pointer via the FDB
entry found in the lookup.
The RTNL assertion assures that the device returned won't disappear.
If the device can disappear, the spinlock added by this patch doesn't
change that at all.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-30 16:42 ` David Miller
0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2018-05-30 16:42 UTC (permalink / raw)
To: petrm; +Cc: netdev, bridge
From: Petr Machata <petrm@mellanox.com>
Date: Mon, 28 May 2018 17:44:16 +0200
> Callers of br_fdb_find() need to hold the hash lock, which
> br_fdb_find_port() doesn't do. Add the missing lock/unlock
> pair.
>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
If all of the these uses of br_fdb_find_port() are safe, then it
should use the RCU fdb lookup variant.
So I basically agree with Stephen that this locking doesn't make any
sense.
The lock is needed when you are going to add or delete an FDB entry.
Here we are doing a lookup and returning a device pointer via the FDB
entry found in the lookup.
The RTNL assertion assures that the device returned won't disappear.
If the device can disappear, the spinlock added by this patch doesn't
change that at all.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Bridge] [PATCH net-next] net: bridge: Lock before br_fdb_find()
2018-05-30 16:42 ` David Miller
@ 2018-05-30 22:03 ` Petr Machata
-1 siblings, 0 replies; 14+ messages in thread
From: Petr Machata @ 2018-05-30 22:03 UTC (permalink / raw)
To: David Miller; +Cc: netdev, bridge
David Miller <davem@davemloft.net> writes:
> From: Petr Machata <petrm@mellanox.com>
> Date: Mon, 28 May 2018 17:44:16 +0200
>
>> Callers of br_fdb_find() need to hold the hash lock, which
>> br_fdb_find_port() doesn't do. Add the missing lock/unlock
>> pair.
>>
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>
> If all of the these uses of br_fdb_find_port() are safe, then it
> should use the RCU fdb lookup variant.
>
> So I basically agree with Stephen that this locking doesn't make any
> sense.
>
> The lock is needed when you are going to add or delete an FDB entry.
>
> Here we are doing a lookup and returning a device pointer via the FDB
> entry found in the lookup.
>
> The RTNL assertion assures that the device returned won't disappear.
>
> If the device can disappear, the spinlock added by this patch doesn't
> change that at all.
OK, I'll take another look at this.
Thanks,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH net-next] net: bridge: Lock before br_fdb_find()
@ 2018-05-30 22:03 ` Petr Machata
0 siblings, 0 replies; 14+ messages in thread
From: Petr Machata @ 2018-05-30 22:03 UTC (permalink / raw)
To: David Miller; +Cc: bridge, netdev, stephen
David Miller <davem@davemloft.net> writes:
> From: Petr Machata <petrm@mellanox.com>
> Date: Mon, 28 May 2018 17:44:16 +0200
>
>> Callers of br_fdb_find() need to hold the hash lock, which
>> br_fdb_find_port() doesn't do. Add the missing lock/unlock
>> pair.
>>
>> Signed-off-by: Petr Machata <petrm@mellanox.com>
>
> If all of the these uses of br_fdb_find_port() are safe, then it
> should use the RCU fdb lookup variant.
>
> So I basically agree with Stephen that this locking doesn't make any
> sense.
>
> The lock is needed when you are going to add or delete an FDB entry.
>
> Here we are doing a lookup and returning a device pointer via the FDB
> entry found in the lookup.
>
> The RTNL assertion assures that the device returned won't disappear.
>
> If the device can disappear, the spinlock added by this patch doesn't
> change that at all.
OK, I'll take another look at this.
Thanks,
Petr
^ permalink raw reply [flat|nested] 14+ messages in thread