| Message ID | 20210125074416.4056484-1-ivecera@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f0947d0d21b219e03940b9be6628a43445c0de7a |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] team: protect features update by RCU to avoid deadlock | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 25 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Sun, Jan 24, 2021 at 11:44 PM Ivan Vecera <ivecera@redhat.com> wrote: > > Function __team_compute_features() is protected by team->lock > mutex when it is called from team_compute_features() used when > features of an underlying device is changed. This causes > a deadlock when NETDEV_FEAT_CHANGE notifier for underlying device > is fired due to change propagated from team driver (e.g. MTU > change). It's because callbacks like team_change_mtu() or > team_vlan_rx_{add,del}_vid() protect their port list traversal > by team->lock mutex. > > Example (r8169 case where this driver disables TSO for certain MTU > values): > ... > [ 6391.348202] __mutex_lock.isra.6+0x2d0/0x4a0 > [ 6391.358602] team_device_event+0x9d/0x160 [team] > [ 6391.363756] notifier_call_chain+0x47/0x70 > [ 6391.368329] netdev_update_features+0x56/0x60 > [ 6391.373207] rtl8169_change_mtu+0x14/0x50 [r8169] > [ 6391.378457] dev_set_mtu_ext+0xe1/0x1d0 > [ 6391.387022] dev_set_mtu+0x52/0x90 > [ 6391.390820] team_change_mtu+0x64/0xf0 [team] > [ 6391.395683] dev_set_mtu_ext+0xe1/0x1d0 > [ 6391.399963] do_setlink+0x231/0xf50 > ... > > In fact team_compute_features() called from team_device_event() > does not need to be protected by team->lock mutex and rcu_read_lock() > is sufficient there for port list traversal. Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com> In the future, please version your patch so that we can easily find out which is the latest. Thanks.
On Mon, 25 Jan 2021 12:53:22 -0800 Cong Wang <xiyou.wangcong@gmail.com> wrote: > On Sun, Jan 24, 2021 at 11:44 PM Ivan Vecera <ivecera@redhat.com> wrote: > > > > Function __team_compute_features() is protected by team->lock > > mutex when it is called from team_compute_features() used when > > features of an underlying device is changed. This causes > > a deadlock when NETDEV_FEAT_CHANGE notifier for underlying device > > is fired due to change propagated from team driver (e.g. MTU > > change). It's because callbacks like team_change_mtu() or > > team_vlan_rx_{add,del}_vid() protect their port list traversal > > by team->lock mutex. > > > > Example (r8169 case where this driver disables TSO for certain MTU > > values): > > ... > > [ 6391.348202] __mutex_lock.isra.6+0x2d0/0x4a0 > > [ 6391.358602] team_device_event+0x9d/0x160 [team] > > [ 6391.363756] notifier_call_chain+0x47/0x70 > > [ 6391.368329] netdev_update_features+0x56/0x60 > > [ 6391.373207] rtl8169_change_mtu+0x14/0x50 [r8169] > > [ 6391.378457] dev_set_mtu_ext+0xe1/0x1d0 > > [ 6391.387022] dev_set_mtu+0x52/0x90 > > [ 6391.390820] team_change_mtu+0x64/0xf0 [team] > > [ 6391.395683] dev_set_mtu_ext+0xe1/0x1d0 > > [ 6391.399963] do_setlink+0x231/0xf50 > > ... > > > > In fact team_compute_features() called from team_device_event() > > does not need to be protected by team->lock mutex and rcu_read_lock() > > is sufficient there for port list traversal. > > Reviewed-by: Cong Wang <xiyou.wangcong@gmail.com> > > In the future, please version your patch so that we can easily > find out which is the latest. OK, got it... I haven't added a version due to different subjects... Thanks, Ivan
Mon, Jan 25, 2021 at 08:44:16AM CET, ivecera@redhat.com wrote: >Function __team_compute_features() is protected by team->lock >mutex when it is called from team_compute_features() used when >features of an underlying device is changed. This causes >a deadlock when NETDEV_FEAT_CHANGE notifier for underlying device >is fired due to change propagated from team driver (e.g. MTU >change). It's because callbacks like team_change_mtu() or >team_vlan_rx_{add,del}_vid() protect their port list traversal >by team->lock mutex. > >Example (r8169 case where this driver disables TSO for certain MTU >values): >... >[ 6391.348202] __mutex_lock.isra.6+0x2d0/0x4a0 >[ 6391.358602] team_device_event+0x9d/0x160 [team] >[ 6391.363756] notifier_call_chain+0x47/0x70 >[ 6391.368329] netdev_update_features+0x56/0x60 >[ 6391.373207] rtl8169_change_mtu+0x14/0x50 [r8169] >[ 6391.378457] dev_set_mtu_ext+0xe1/0x1d0 >[ 6391.387022] dev_set_mtu+0x52/0x90 >[ 6391.390820] team_change_mtu+0x64/0xf0 [team] >[ 6391.395683] dev_set_mtu_ext+0xe1/0x1d0 >[ 6391.399963] do_setlink+0x231/0xf50 >... > >In fact team_compute_features() called from team_device_event() >does not need to be protected by team->lock mutex and rcu_read_lock() >is sufficient there for port list traversal. > >Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device") >Cc: Jiri Pirko <jiri@resnulli.us> >Cc: David S. Miller <davem@davemloft.net> >Cc: Cong Wang <xiyou.wangcong@gmail.com> >Cc: Jakub Kicinski <kuba@kernel.org> >Cc: Saeed Mahameed <saeed@kernel.org> >Signed-off-by: Ivan Vecera <ivecera@redhat.com> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Mon, 25 Jan 2021 08:44:16 +0100 you wrote: > Function __team_compute_features() is protected by team->lock > mutex when it is called from team_compute_features() used when > features of an underlying device is changed. This causes > a deadlock when NETDEV_FEAT_CHANGE notifier for underlying device > is fired due to change propagated from team driver (e.g. MTU > change). It's because callbacks like team_change_mtu() or > team_vlan_rx_{add,del}_vid() protect their port list traversal > by team->lock mutex. > > [...] Here is the summary with links: - [net] team: protect features update by RCU to avoid deadlock https://git.kernel.org/netdev/net/c/f0947d0d21b2 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c index c19dac21c468..dd7917cab2b1 100644 --- a/drivers/net/team/team.c +++ b/drivers/net/team/team.c @@ -992,7 +992,8 @@ static void __team_compute_features(struct team *team) unsigned int dst_release_flag = IFF_XMIT_DST_RELEASE | IFF_XMIT_DST_RELEASE_PERM; - list_for_each_entry(port, &team->port_list, list) { + rcu_read_lock(); + list_for_each_entry_rcu(port, &team->port_list, list) { vlan_features = netdev_increment_features(vlan_features, port->dev->vlan_features, TEAM_VLAN_FEATURES); @@ -1006,6 +1007,7 @@ static void __team_compute_features(struct team *team) if (port->dev->hard_header_len > max_hard_header_len) max_hard_header_len = port->dev->hard_header_len; } + rcu_read_unlock(); team->dev->vlan_features = vlan_features; team->dev->hw_enc_features = enc_features | NETIF_F_GSO_ENCAP_ALL | @@ -1020,9 +1022,7 @@ static void __team_compute_features(struct team *team) static void team_compute_features(struct team *team) { - mutex_lock(&team->lock); __team_compute_features(team); - mutex_unlock(&team->lock); netdev_change_features(team->dev); }
Function __team_compute_features() is protected by team->lock mutex when it is called from team_compute_features() used when features of an underlying device is changed. This causes a deadlock when NETDEV_FEAT_CHANGE notifier for underlying device is fired due to change propagated from team driver (e.g. MTU change). It's because callbacks like team_change_mtu() or team_vlan_rx_{add,del}_vid() protect their port list traversal by team->lock mutex. Example (r8169 case where this driver disables TSO for certain MTU values): ... [ 6391.348202] __mutex_lock.isra.6+0x2d0/0x4a0 [ 6391.358602] team_device_event+0x9d/0x160 [team] [ 6391.363756] notifier_call_chain+0x47/0x70 [ 6391.368329] netdev_update_features+0x56/0x60 [ 6391.373207] rtl8169_change_mtu+0x14/0x50 [r8169] [ 6391.378457] dev_set_mtu_ext+0xe1/0x1d0 [ 6391.387022] dev_set_mtu+0x52/0x90 [ 6391.390820] team_change_mtu+0x64/0xf0 [team] [ 6391.395683] dev_set_mtu_ext+0xe1/0x1d0 [ 6391.399963] do_setlink+0x231/0xf50 ... In fact team_compute_features() called from team_device_event() does not need to be protected by team->lock mutex and rcu_read_lock() is sufficient there for port list traversal. Fixes: 3d249d4ca7d0 ("net: introduce ethernet teaming device") Cc: Jiri Pirko <jiri@resnulli.us> Cc: David S. Miller <davem@davemloft.net> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Saeed Mahameed <saeed@kernel.org> Signed-off-by: Ivan Vecera <ivecera@redhat.com> --- drivers/net/team/team.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)