* [PATCH net,v2] team: fix possible deadlock in team_port_change_check
@ 2024-08-02 14:59 Jeongjun Park
2024-08-02 15:08 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Jeongjun Park @ 2024-08-02 14:59 UTC (permalink / raw)
To: jiri
Cc: davem, edumazet, kuba, pabeni, netdev, linux-kernel,
syzbot+b668da2bc4cb9670bf58, Jeongjun Park
In do_setlink() , do_set_master() is called when dev->flags does not have
the IFF_UP flag set, so 'team->lock' is acquired and dev_open() is called,
which generates the NETDEV_UP event. This causes a deadlock as it tries to
acquire 'team->lock' again.
To fix this, we need to remove the unnecessary mutex_lock from all paths
protected by rtnl. This patch also includes patches for paths that are
already protected by rtnl and do not need the mutex_lock, in addition
to the root cause reported.
Reported-by: syzbot+b668da2bc4cb9670bf58@syzkaller.appspotmail.com
Fixes: 61dc3461b954 ("team: convert overall spinlock to mutex")
Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
drivers/net/team/team_core.c | 22 ----------------------
1 file changed, 22 deletions(-)
diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index ab1935a4aa2c..f7bab2d2a281 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -1668,7 +1668,6 @@ static void team_uninit(struct net_device *dev)
struct team_port *port;
struct team_port *tmp;
- mutex_lock(&team->lock);
list_for_each_entry_safe(port, tmp, &team->port_list, list)
team_port_del(team, port->dev);
@@ -1677,7 +1676,6 @@ static void team_uninit(struct net_device *dev)
team_mcast_rejoin_fini(team);
team_notify_peers_fini(team);
team_queue_override_fini(team);
- mutex_unlock(&team->lock);
netdev_change_features(dev);
lockdep_unregister_key(&team->team_lock_key);
}
@@ -1818,7 +1816,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
* Alhough this is reader, it's guarded by team lock. It's not possible
* to traverse list in reverse under rcu_read_lock
*/
- mutex_lock(&team->lock);
team->port_mtu_change_allowed = true;
list_for_each_entry(port, &team->port_list, list) {
err = dev_set_mtu(port->dev, new_mtu);
@@ -1829,7 +1826,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
}
}
team->port_mtu_change_allowed = false;
- mutex_unlock(&team->lock);
WRITE_ONCE(dev->mtu, new_mtu);
@@ -1839,7 +1835,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
list_for_each_entry_continue_reverse(port, &team->port_list, list)
dev_set_mtu(port->dev, dev->mtu);
team->port_mtu_change_allowed = false;
- mutex_unlock(&team->lock);
return err;
}
@@ -1893,20 +1888,17 @@ static int team_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
* Alhough this is reader, it's guarded by team lock. It's not possible
* to traverse list in reverse under rcu_read_lock
*/
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list) {
err = vlan_vid_add(port->dev, proto, vid);
if (err)
goto unwind;
}
- mutex_unlock(&team->lock);
return 0;
unwind:
list_for_each_entry_continue_reverse(port, &team->port_list, list)
vlan_vid_del(port->dev, proto, vid);
- mutex_unlock(&team->lock);
return err;
}
@@ -1916,10 +1908,8 @@ static int team_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
struct team *team = netdev_priv(dev);
struct team_port *port;
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list)
vlan_vid_del(port->dev, proto, vid);
- mutex_unlock(&team->lock);
return 0;
}
@@ -1941,9 +1931,7 @@ static void team_netpoll_cleanup(struct net_device *dev)
{
struct team *team = netdev_priv(dev);
- mutex_lock(&team->lock);
__team_netpoll_cleanup(team);
- mutex_unlock(&team->lock);
}
static int team_netpoll_setup(struct net_device *dev,
@@ -1953,7 +1941,6 @@ static int team_netpoll_setup(struct net_device *dev,
struct team_port *port;
int err = 0;
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list) {
err = __team_port_enable_netpoll(port);
if (err) {
@@ -1961,7 +1948,6 @@ static int team_netpoll_setup(struct net_device *dev,
break;
}
}
- mutex_unlock(&team->lock);
return err;
}
#endif
@@ -1972,9 +1958,7 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev,
struct team *team = netdev_priv(dev);
int err;
- mutex_lock(&team->lock);
err = team_port_add(team, port_dev, extack);
- mutex_unlock(&team->lock);
if (!err)
netdev_change_features(dev);
@@ -1987,9 +1971,7 @@ static int team_del_slave(struct net_device *dev, struct net_device *port_dev)
struct team *team = netdev_priv(dev);
int err;
- mutex_lock(&team->lock);
err = team_port_del(team, port_dev);
- mutex_unlock(&team->lock);
if (err)
return err;
@@ -2945,11 +2927,7 @@ static void __team_port_change_port_removed(struct team_port *port)
static void team_port_change_check(struct team_port *port, bool linkup)
{
- struct team *team = port->team;
-
- mutex_lock(&team->lock);
__team_port_change_check(port, linkup);
- mutex_unlock(&team->lock);
}
--
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-02 14:59 [PATCH net,v2] team: fix possible deadlock in team_port_change_check Jeongjun Park
@ 2024-08-02 15:08 ` Eric Dumazet
2024-08-02 16:25 ` Jeongjun Park
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2024-08-02 15:08 UTC (permalink / raw)
To: Jeongjun Park
Cc: jiri, davem, kuba, pabeni, netdev, linux-kernel,
syzbot+b668da2bc4cb9670bf58
On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
>
> In do_setlink() , do_set_master() is called when dev->flags does not have
> the IFF_UP flag set, so 'team->lock' is acquired and dev_open() is called,
> which generates the NETDEV_UP event. This causes a deadlock as it tries to
> acquire 'team->lock' again.
>
> To fix this, we need to remove the unnecessary mutex_lock from all paths
> protected by rtnl. This patch also includes patches for paths that are
> already protected by rtnl and do not need the mutex_lock, in addition
> to the root cause reported.
>
> Reported-by: syzbot+b668da2bc4cb9670bf58@syzkaller.appspotmail.com
> Fixes: 61dc3461b954 ("team: convert overall spinlock to mutex")
> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> ---
This can not be right.
You have to completely remove team->lock, otherwise paths not taking
RTNL will race with ones holding RTNL.
Add ASSERT_RTNL() at the places we had a mutex_lock(&team->lock), and
see how it goes.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-02 15:08 ` Eric Dumazet
@ 2024-08-02 16:25 ` Jeongjun Park
2024-08-02 17:42 ` Jiri Pirko
0 siblings, 1 reply; 7+ messages in thread
From: Jeongjun Park @ 2024-08-02 16:25 UTC (permalink / raw)
To: edumazet
Cc: aha310510, davem, jiri, kuba, linux-kernel, netdev, pabeni,
syzbot+b668da2bc4cb9670bf58
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 8347 bytes --]
Eric Dumazet wrote:
>
> On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
> >
> > In do_setlink() , do_set_master() is called when dev->flags does not have
> > the IFF_UP flag set, so 'team->lock' is acquired and dev_open() is called,
> > which generates the NETDEV_UP event. This causes a deadlock as it tries to
> > acquire 'team->lock' again.
> >
> > To fix this, we need to remove the unnecessary mutex_lock from all paths
> > protected by rtnl. This patch also includes patches for paths that are
> > already protected by rtnl and do not need the mutex_lock, in addition
> > to the root cause reported.
> >
> > Reported-by: syzbot+b668da2bc4cb9670bf58@syzkaller.appspotmail.com
> > Fixes: 61dc3461b954 ("team: convert overall spinlock to mutex")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
>
> This can not be right.
>
> You have to completely remove team->lock, otherwise paths not taking
> RTNL will race with ones holding RTNL.
>
> Add ASSERT_RTNL() at the places we had a mutex_lock(&team->lock), and
> see how it goes.
Thanks for the advice. I've rewritten the patch to remove all mutex_lock
as you suggested. I've also added checks for specific paths that don't use
rtnl_lock. What do you think about patching it this way?
Regards,
Jeongjun Park
---
drivers/net/team/team_core.c | 47 +++++++++---------------------------
include/linux/if_team.h | 4 +--
2 files changed, 12 insertions(+), 39 deletions(-)
diff --git a/drivers/net/team/team_core.c b/drivers/net/team/team_core.c
index ab1935a4aa2c..1e2c631e3b78 100644
--- a/drivers/net/team/team_core.c
+++ b/drivers/net/team/team_core.c
@@ -1621,6 +1621,7 @@ static int team_init(struct net_device *dev)
team->dev = dev;
team_set_no_mode(team);
team->notifier_ctx = false;
+ team->rtnl_locked = false;
team->pcpu_stats = netdev_alloc_pcpu_stats(struct team_pcpu_stats);
if (!team->pcpu_stats)
@@ -1646,10 +1647,6 @@ static int team_init(struct net_device *dev)
goto err_options_register;
netif_carrier_off(dev);
- lockdep_register_key(&team->team_lock_key);
- __mutex_init(&team->lock, "team->team_lock_key", &team->team_lock_key);
- netdev_lockdep_set_classes(dev);
-
return 0;
err_options_register:
@@ -1668,7 +1665,6 @@ static void team_uninit(struct net_device *dev)
struct team_port *port;
struct team_port *tmp;
- mutex_lock(&team->lock);
list_for_each_entry_safe(port, tmp, &team->port_list, list)
team_port_del(team, port->dev);
@@ -1677,9 +1673,7 @@ static void team_uninit(struct net_device *dev)
team_mcast_rejoin_fini(team);
team_notify_peers_fini(team);
team_queue_override_fini(team);
- mutex_unlock(&team->lock);
netdev_change_features(dev);
- lockdep_unregister_key(&team->team_lock_key);
}
static void team_destructor(struct net_device *dev)
@@ -1800,11 +1794,9 @@ static int team_set_mac_address(struct net_device *dev, void *p)
if (dev->type == ARPHRD_ETHER && !is_valid_ether_addr(addr->sa_data))
return -EADDRNOTAVAIL;
dev_addr_set(dev, addr->sa_data);
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list)
if (team->ops.port_change_dev_addr)
team->ops.port_change_dev_addr(team, port);
- mutex_unlock(&team->lock);
return 0;
}
@@ -1818,7 +1810,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
* Alhough this is reader, it's guarded by team lock. It's not possible
* to traverse list in reverse under rcu_read_lock
*/
- mutex_lock(&team->lock);
team->port_mtu_change_allowed = true;
list_for_each_entry(port, &team->port_list, list) {
err = dev_set_mtu(port->dev, new_mtu);
@@ -1829,7 +1820,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
}
}
team->port_mtu_change_allowed = false;
- mutex_unlock(&team->lock);
WRITE_ONCE(dev->mtu, new_mtu);
@@ -1839,7 +1829,6 @@ static int team_change_mtu(struct net_device *dev, int new_mtu)
list_for_each_entry_continue_reverse(port, &team->port_list, list)
dev_set_mtu(port->dev, dev->mtu);
team->port_mtu_change_allowed = false;
- mutex_unlock(&team->lock);
return err;
}
@@ -1893,20 +1882,17 @@ static int team_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid)
* Alhough this is reader, it's guarded by team lock. It's not possible
* to traverse list in reverse under rcu_read_lock
*/
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list) {
err = vlan_vid_add(port->dev, proto, vid);
if (err)
goto unwind;
}
- mutex_unlock(&team->lock);
return 0;
unwind:
list_for_each_entry_continue_reverse(port, &team->port_list, list)
vlan_vid_del(port->dev, proto, vid);
- mutex_unlock(&team->lock);
return err;
}
@@ -1916,10 +1902,8 @@ static int team_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
struct team *team = netdev_priv(dev);
struct team_port *port;
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list)
vlan_vid_del(port->dev, proto, vid);
- mutex_unlock(&team->lock);
return 0;
}
@@ -1941,9 +1925,7 @@ static void team_netpoll_cleanup(struct net_device *dev)
{
struct team *team = netdev_priv(dev);
- mutex_lock(&team->lock);
__team_netpoll_cleanup(team);
- mutex_unlock(&team->lock);
}
static int team_netpoll_setup(struct net_device *dev,
@@ -1953,7 +1935,6 @@ static int team_netpoll_setup(struct net_device *dev,
struct team_port *port;
int err = 0;
- mutex_lock(&team->lock);
list_for_each_entry(port, &team->port_list, list) {
err = __team_port_enable_netpoll(port);
if (err) {
@@ -1961,7 +1942,6 @@ static int team_netpoll_setup(struct net_device *dev,
break;
}
}
- mutex_unlock(&team->lock);
return err;
}
#endif
@@ -1972,9 +1952,7 @@ static int team_add_slave(struct net_device *dev, struct net_device *port_dev,
struct team *team = netdev_priv(dev);
int err;
- mutex_lock(&team->lock);
err = team_port_add(team, port_dev, extack);
- mutex_unlock(&team->lock);
if (!err)
netdev_change_features(dev);
@@ -1987,18 +1965,11 @@ static int team_del_slave(struct net_device *dev, struct net_device *port_dev)
struct team *team = netdev_priv(dev);
int err;
- mutex_lock(&team->lock);
err = team_port_del(team, port_dev);
- mutex_unlock(&team->lock);
if (err)
return err;
- if (netif_is_team_master(port_dev)) {
- lockdep_unregister_key(&team->team_lock_key);
- lockdep_register_key(&team->team_lock_key);
- lockdep_set_class(&team->lock, &team->team_lock_key);
- }
netdev_change_features(dev);
return err;
@@ -2305,13 +2276,11 @@ static struct team *team_nl_team_get(struct genl_info *info)
}
team = netdev_priv(dev);
- mutex_lock(&team->lock);
return team;
}
static void team_nl_team_put(struct team *team)
{
- mutex_unlock(&team->lock);
dev_put(team->dev);
}
@@ -2501,6 +2470,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
int err;
LIST_HEAD(sel_opt_inst_list);
+ if (!rtnl_is_locked()) {
+ rtnl_lock();
+ team->rtnl_locked = true;
+ }
+
team = team_nl_team_get(info);
if (!team)
return -EINVAL;
@@ -2513,6 +2487,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
team_nl_team_put(team);
+ if (team->rtnl_locked) {
+ team->rtnl_locked = false;
+ rtnl_unlock();
+ }
+
return err;
}
@@ -2945,11 +2924,7 @@ static void __team_port_change_port_removed(struct team_port *port)
static void team_port_change_check(struct team_port *port, bool linkup)
{
- struct team *team = port->team;
-
- mutex_lock(&team->lock);
__team_port_change_check(port, linkup);
- mutex_unlock(&team->lock);
}
diff --git a/include/linux/if_team.h b/include/linux/if_team.h
index cdc684e04a2f..930c5deb4ad1 100644
--- a/include/linux/if_team.h
+++ b/include/linux/if_team.h
@@ -191,8 +191,6 @@ struct team {
const struct header_ops *header_ops_cache;
- struct mutex lock; /* used for overall locking, e.g. port lists write */
-
/*
* List of enabled ports and their count
*/
@@ -223,8 +221,8 @@ struct team {
atomic_t count_pending;
struct delayed_work dw;
} mcast_rejoin;
- struct lock_class_key team_lock_key;
long mode_priv[TEAM_MODE_PRIV_LONGS];
+ bool rtnl_locked;
};
static inline int team_dev_queue_xmit(struct team *team, struct team_port *port,
--
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-02 16:25 ` Jeongjun Park
@ 2024-08-02 17:42 ` Jiri Pirko
2024-08-03 1:36 ` Jeongjun Park
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Pirko @ 2024-08-02 17:42 UTC (permalink / raw)
To: Jeongjun Park
Cc: edumazet, davem, kuba, linux-kernel, netdev, pabeni,
syzbot+b668da2bc4cb9670bf58
Fri, Aug 02, 2024 at 06:25:31PM CEST, aha310510@gmail.com wrote:
>Eric Dumazet wrote:
>>
>> On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
>> >
[..]
>@@ -2501,6 +2470,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
> int err;
> LIST_HEAD(sel_opt_inst_list);
>
>+ if (!rtnl_is_locked()) {
This is completely wrong, other thread may hold the lock.
>+ rtnl_lock();
NACK! I wrote it in the other thread. Don't take rtnl for get options
command. It is used for repeated fetch of stats. It's read only. Should
be converted to RCU.
Why are you so obsessed by this hypothetical syzcaller bug? Are you
hitting this in real? If not, please let it go. I will fix it myself
when I find some spare cycles.
>+ team->rtnl_locked = true;
>+ }
>+
> team = team_nl_team_get(info);
> if (!team)
> return -EINVAL;
>@@ -2513,6 +2487,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
>
> team_nl_team_put(team);
>
>+ if (team->rtnl_locked) {
>+ team->rtnl_locked = false;
>+ rtnl_unlock();
>+ }
>+
> return err;
> }
>
[..]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-02 17:42 ` Jiri Pirko
@ 2024-08-03 1:36 ` Jeongjun Park
2024-08-03 6:58 ` Jiri Pirko
0 siblings, 1 reply; 7+ messages in thread
From: Jeongjun Park @ 2024-08-03 1:36 UTC (permalink / raw)
To: Jiri Pirko
Cc: edumazet, davem, kuba, linux-kernel, netdev, pabeni,
syzbot+b668da2bc4cb9670bf58
> Jiri Pirko wrote:
>
> Fri, Aug 02, 2024 at 06:25:31PM CEST, aha310510@gmail.com wrote:
>> Eric Dumazet wrote:
>>>
>>>> On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
>>>>>
>>
>> [..]
>>
>> @@ -2501,6 +2470,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
>> int err;
>> LIST_HEAD(sel_opt_inst_list);
>>
>> + if (!rtnl_is_locked()) {
>
> This is completely wrong, other thread may hold the lock.
>
>
>> + rtnl_lock();
>
> NACK! I wrote it in the other thread. Don't take rtnl for get options
> command. It is used for repeated fetch of stats. It's read only. Should
> be converted to RCU.
>
I see. But, in the current, when called through the following path:
team_nl_send_event_options_get()->
team_nl_send_options_get()->
team_nl_fill_one_option_get()
, it was protected through rtnl. Does this mean that rcu should be
used instead of rtnl in this case as well?
> Why are you so obsessed by this hypothetical syzcaller bug? Are you
> hitting this in real? If not, please let it go. I will fix it myself
> when I find some spare cycles.
Sorry for the inconvenience, but I don't want to give up on this bug
so easily since it is a valid bug that we have started analyzing
anyway and the direction of how to fix it is clear. I hope you
understand and I will send you a patch that uses rcu instead
of rtnl soon.
Regards,
Jeongjun Park
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-03 1:36 ` Jeongjun Park
@ 2024-08-03 6:58 ` Jiri Pirko
2024-08-03 11:51 ` Jeongjun Park
0 siblings, 1 reply; 7+ messages in thread
From: Jiri Pirko @ 2024-08-03 6:58 UTC (permalink / raw)
To: Jeongjun Park
Cc: edumazet, davem, kuba, linux-kernel, netdev, pabeni,
syzbot+b668da2bc4cb9670bf58
Sat, Aug 03, 2024 at 03:36:48AM CEST, aha310510@gmail.com wrote:
>
>
>> Jiri Pirko wrote:
>>
>> Fri, Aug 02, 2024 at 06:25:31PM CEST, aha310510@gmail.com wrote:
>>> Eric Dumazet wrote:
>>>>
>>>>> On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
>>>>>>
>>>
>>> [..]
>>>
>>> @@ -2501,6 +2470,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
>>> int err;
>>> LIST_HEAD(sel_opt_inst_list);
>>>
>>> + if (!rtnl_is_locked()) {
>>
>> This is completely wrong, other thread may hold the lock.
>>
>>
>>> + rtnl_lock();
>>
>> NACK! I wrote it in the other thread. Don't take rtnl for get options
>> command. It is used for repeated fetch of stats. It's read only. Should
>> be converted to RCU.
>>
>
>I see. But, in the current, when called through the following path:
>team_nl_send_event_options_get()->
>team_nl_send_options_get()->
>team_nl_fill_one_option_get()
>, it was protected through rtnl. Does this mean that rcu should be
Not true. team_nl_send_event_options_get() is sometimes called without
rtnl (from lb_stats_refresh() for example).
>used instead of rtnl in this case as well?
>
>> Why are you so obsessed by this hypothetical syzcaller bug? Are you
>> hitting this in real? If not, please let it go. I will fix it myself
>> when I find some spare cycles.
>
>Sorry for the inconvenience, but I don't want to give up on this bug
>so easily since it is a valid bug that we have started analyzing
>anyway and the direction of how to fix it is clear. I hope you
>understand and I will send you a patch that uses rcu instead
I don't understand, sorry. Apparently you don't have any clue what you
are doing and only waste people time. Can't you find another toy to
play?
Please add my
Nacked-by: Jiri Pirko <jiri@nvidia.com>
tag to your any future attempt, as I'm sure it won't be correct.
Thanks!
>of rtnl soon.
>
>Regards,
>Jeongjun Park
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net,v2] team: fix possible deadlock in team_port_change_check
2024-08-03 6:58 ` Jiri Pirko
@ 2024-08-03 11:51 ` Jeongjun Park
0 siblings, 0 replies; 7+ messages in thread
From: Jeongjun Park @ 2024-08-03 11:51 UTC (permalink / raw)
To: Jiri Pirko
Cc: edumazet, davem, kuba, linux-kernel, netdev, pabeni,
syzbot+b668da2bc4cb9670bf58
Jiri Pirko wrote:
>
> Sat, Aug 03, 2024 at 03:36:48AM CEST, aha310510@gmail.com wrote:
> >
> >
> >> Jiri Pirko wrote:
> >>
> >> Fri, Aug 02, 2024 at 06:25:31PM CEST, aha310510@gmail.com wrote:
> >>> Eric Dumazet wrote:
> >>>>
> >>>>> On Fri, Aug 2, 2024 at 5:00 PM Jeongjun Park <aha310510@gmail.com> wrote:
> >>>>>>
> >>>
> >>> [..]
> >>>
> >>> @@ -2501,6 +2470,11 @@ int team_nl_options_get_doit(struct sk_buff *skb, struct genl_info *info)
> >>> int err;
> >>> LIST_HEAD(sel_opt_inst_list);
> >>>
> >>> + if (!rtnl_is_locked()) {
> >>
> >> This is completely wrong, other thread may hold the lock.
> >>
> >>
> >>> + rtnl_lock();
> >>
> >> NACK! I wrote it in the other thread. Don't take rtnl for get options
> >> command. It is used for repeated fetch of stats. It's read only. Should
> >> be converted to RCU.
> >>
> >
> >I see. But, in the current, when called through the following path:
> >team_nl_send_event_options_get()->
> >team_nl_send_options_get()->
> >team_nl_fill_one_option_get()
> >, it was protected through rtnl. Does this mean that rcu should be
>
> Not true. team_nl_send_event_options_get() is sometimes called without
> rtnl (from lb_stats_refresh() for example).
>
>
> >used instead of rtnl in this case as well?
> >
> >> Why are you so obsessed by this hypothetical syzcaller bug? Are you
> >> hitting this in real? If not, please let it go. I will fix it myself
> >> when I find some spare cycles.
> >
> >Sorry for the inconvenience, but I don't want to give up on this bug
> >so easily since it is a valid bug that we have started analyzing
> >anyway and the direction of how to fix it is clear. I hope you
> >understand and I will send you a patch that uses rcu instead
>
> I don't understand, sorry. Apparently you don't have any clue what you
> are doing and only waste people time. Can't you find another toy to
> play?
>
> Please add my
> Nacked-by: Jiri Pirko <jiri@nvidia.com>
> tag to your any future attempt, as I'm sure it won't be correct.
>
I am truly sorry. I feel like I wasted people time by sending meaningless
patches repeatedly, approaching this issue recklessly. I will be more
careful in my analysis and sending emails in the future.
Regards,
Jeongjun Park
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-08-03 11:51 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-02 14:59 [PATCH net,v2] team: fix possible deadlock in team_port_change_check Jeongjun Park
2024-08-02 15:08 ` Eric Dumazet
2024-08-02 16:25 ` Jeongjun Park
2024-08-02 17:42 ` Jiri Pirko
2024-08-03 1:36 ` Jeongjun Park
2024-08-03 6:58 ` Jiri Pirko
2024-08-03 11:51 ` Jeongjun Park
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox