Re: [PATCH net v4] team: fix null-ptr-deref when team device type is changed

["Ziyang Xuan (William)" <william.xuanziyang@huawei.com>]

> On Wed, 2023-09-13 at 14:15 +0800, Ziyang Xuan (William) wrote:
> 
> To me both cases look the same in the end: the team driver sets and use
> header_ops of a different device that will assume dev_priv() being a
> different struct.
> 
> I'm guessing a generic solution could be implementing 'trampoline'
> header_ops that just call into the lower port corresponding op, and
> assigning such ops to the team device every time the lower has non
> ethernet header_ops.
> 
> team_dev_type_check_change() should then probably check both dev->type
> and dev->header_ops.
> 
>>> Exporting 'eth_header_ops' for team's sake only looks a bit too
>>> much to
>>> me. I think could instead cache the header_ops ptr after the
>>> initial
>>> ether_setup().
>>
>> Is it possible to use ether_setup() like bonding driver andmodify MTU
>> individually later?
> 
> That could be another option to get the eth_header_ops.
> 
> Note that in the end both are quite similar, you will have to cache
> some info (the mtu with the latter); ether_setup() possibly will have
> more side effects, as it touches many fields. I personally would use
> the thing I suggested above.
> 
Hi Pallo,

Is it possible to modify like this?

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index d3dc22509ea5..8e6a87ba85aa 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2127,7 +2127,12 @@ static const struct ethtool_ops team_ethtool_ops = {
 static void team_setup_by_port(struct net_device *dev,
                               struct net_device *port_dev)
 {
-       dev->header_ops = port_dev->header_ops;
+       struct team *team = netdev_priv(dev);
+
+       if (port_dev->type == ARPHRD_ETHER)
+               dev->header_ops = team->header_ops_cache;
+       else
+               dev->header_ops = port_dev->header_ops;
        dev->type = port_dev->type;
        dev->hard_header_len = port_dev->hard_header_len;
        dev->needed_headroom = port_dev->needed_headroom;
@@ -2174,8 +2179,11 @@ static int team_dev_type_check_change(struct net_device *dev,

 static void team_setup(struct net_device *dev)
 {
+       struct team *team = netdev_priv(dev);
+
        ether_setup(dev);
        dev->max_mtu = ETH_MAX_MTU;
+       team->header_ops_cache = dev->header_ops;

        dev->netdev_ops = &team_netdev_ops;
        dev->ethtool_ops = &team_ethtool_ops;
diff --git a/include/linux/if_team.h b/include/linux/if_team.h
index 8de6b6e67829..34bcba5a7067 100644
--- a/include/linux/if_team.h
+++ b/include/linux/if_team.h
@@ -189,6 +189,8 @@ struct team {
        struct net_device *dev; /* associated netdevice */
        struct team_pcpu_stats __percpu *pcpu_stats;

+       const struct header_ops *header_ops_cache;
+
        struct mutex lock; /* used for overall locking, e.g. port lists write */


Thank you.

> Cheers,
> 
> Paolo
> 
> .
>