Re: [PATCH net] net: do not bond/team netdevices which use ml_priv

[Oliver Hartkopp <socketcan@hartkopp.net>]

Hi Alexandra,

On 20.08.26 10:29, Alexandra Winter wrote:
> 
> 
> On 19.08.26 11:12, Hangbin Liu wrote:
>>> The fact that the issues have been found by syzbot for CAN devices might be
>>> through to the fact that the virtual CAN interface (vcan) can be created by
>>> netlink commands and can be easily used in test setups.
>>>
>>> So what would happen, if the same tests with bonding/teaming/tunneling would
>>> be done with real hardware drivers as the mentioned "direct ml_priv writers
>>> still in tree at this revision:
>>>
>>>    drivers/s390/net/qeth_core_main.c:qeth_alloc_netdev()
>>>        dev->ml_priv = card;
>>>    drivers/net/ethernet/chelsio/cxgb/cxgb2.c:init_one()
>>>        netdev->ml_priv = adapter;
>>>    drivers/net/wan/hdlc_fr.c:fr_add_pvc()
>>>        dev->ml_priv = pvc;
>>>
>>> plus drivers/net/ethernet/i825xx/82596.c, drivers/s390/net/ctcm_main.c,
>>> the libertas main.c/mesh.c paths and
>>> drivers/net/wireless/microchip/wilc1000/netdev.c."
>>>
>>> ??
>> I'm not worry about cxgb2 or 82596, which are too old. But s390 qeth is
>> still actively maintained (Cc the maintainers). Can we block them directly?
> 
> 
> Thank you very much for the Cc I would have missed this otherwise.
> While drivers/s390/net/qeth may be decades old, it is still the most used
> network driver for the s390 architecture.
> 
> !!
> qeth_l2 is an ethernet driver and bonding is heavily used by our customers.
> So: No, please do NOT block bonding over qeth.

Agreed.

> I see your discussion has moved on to other options, but I wanted to point that out.
> 
> 
> 
> qeth_l3 is a transport layer driver (arp offloaded), so I don't think bonding or teaming
> can work at all there.
> ctcm is not based on ethernet, so I don't think bonding or teaming can work there neither.
> Aswin and I will put it on our ToDo list to find out what happens, if somebody tries.
> Maybe we to add them to the blacklist you mention in a later reply?
>

Ack.

> 
>>
>>> If bonding/teaming/tunneling might accidentally overwrite dev->ml_priv we
>>> have to block all those devices. No matter if it is CAN or whatever ethernet
>>> device.
>> How would bonding modify dev->ml_priv?
> 
> 
> Could you give more information about this?
> I hope there is no issue for qeth_l2. There we use and rely on dev->ml_priv.
> 

I did some more investigation on all this.

There are 5 drivers that are using ml_priv:

- qeth
- ctcm
- cxgb2
- i596
- wilc1000

where qeth and i596 are using it to store a single pointer which can 
also be done by adding this pointer to their netdev_priv structure.

wilc1000 assigns ml_priv and never reads from it (development leftover).

Only cxgb2 and ctcm use it in a more complex way that would make it 
tricky to move its functionality into netdev_priv without having real 
hardware on the desk.

Either team and bonding do not fiddle with ml_priv on their own. But 
they make assumptions that best fit to ethernet devices where they don't 
care about nor copy any ml_priv pointers.

This caused a problem on CAN devices that were not created by the CAN 
driver infrastructure (creating proper ml_priv content). When TUN/TAP 
set the dev->type of an ethernet device to ARPHRD_CAN the CAN ml_priv is 
NULL (not initialized).

Long story short:

The ml_priv assignment in wilc100 can be removed.
For ethernet devices like the qeth there's no problem AFAICS.
But I would think about making use of netdev_priv() there:

diff --git a/drivers/s390/net/qeth_core.h b/drivers/s390/net/qeth_core.h
index 41fe8a0..b21bccc 100644
--- a/drivers/s390/net/qeth_core.h
+++ b/drivers/s390/net/qeth_core.h
@@ -798,8 +798,20 @@ struct qeth_priv {
         unsigned int tx_wanted_queues;
         u32 brport_hw_features;
         u32 brport_features;
+       struct qeth_card *card;
  };

+static inline struct qeth_card *qeth_dev_get_card(struct net_device *dev)
+{
+       return ((struct qeth_priv *)netdev_priv(dev))->card;
+}
+
+static inline void qeth_dev_set_card(struct net_device *dev,
+                                     struct qeth_card *card)
+{
+       ((struct qeth_priv *)netdev_priv(dev))->card = card;
+}
+

diff --git a/drivers/s390/net/qeth_core_main.c 
b/drivers/s390/net/qeth_core_main.c
index 7376a45..b00fa77 100644
--- a/drivers/s390/net/qeth_core_main.c
+++ b/drivers/s390/net/qeth_core_main.c
@@ -4562,7 +4562,7 @@ void qeth_tx_timeout(struct net_device *dev, 
unsigned int txqueue)
  {
         struct qeth_card *card;

-       card = dev->ml_priv;
+       card = qeth_dev_get_card(dev);
         QETH_CARD_TEXT(card, 4, "txtimeo");
         qeth_schedule_recovery(card);
  }

(..)

A similar easy adoption could be done for i596 too.

ctcm cxgb2 should stay on ml_priv usage but they should implement the 
tagging of ml_priv, e.g.

diff --git a/drivers/s390/net/ctcm_fsms.c b/drivers/s390/net/ctcm_fsms.c
index bf917f4..a1465f3 100644
--- a/drivers/s390/net/ctcm_fsms.c
+++ b/drivers/s390/net/ctcm_fsms.c
@@ -246,7 +246,7 @@ static void chx_txdone(fsm_instance *fi, int event, 
void *arg)
  {
         struct channel *ch = arg;
         struct net_device *dev = ch->netdev;
-       struct ctcm_priv *priv = dev->ml_priv;
+       struct ctcm_priv *priv = netdev_get_ml_priv(dev, ML_PRIV_CTCM);
         struct sk_buff *skb;
         int first = 1;
         int i;

(..)

@@ -1097,7 +1097,7 @@ static struct net_device 
*ctcm_init_netdevice(struct ctcm_priv *priv)
                         CTCM_FUNTAIL);
                 return NULL;
         }
-       dev->ml_priv = priv;
+       netdev_set_ml_priv(dev, priv, ML_PRIV_CTCM);
         priv->fsm = init_fsm("ctcmdev", dev_state_names, dev_event_names,


The final question (which is not really a ml_priv issue) is how to tell 
team/bonding which netdevices are not capable to be used by them. To 
cover e.g. your ctcm driver using ARPHRD_SLIP.

The current check (bond_dev->type != slave_dev->type) would allow to 
join two type-identical interfaces, which was at least not a good idea 
for CAN. For that reason we already check (slave_dev->type == 
ARPHRD_CAN) there. Other dev->types might follow.

Not sure if collecting a bunch of ARPHRD values is the right approach or 
whether team/bonding should check required features and settings (like 
IFF flags, e.g. IFF_ARP or specific address length)?

Best regards,
Oliver