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

Hi Alexandra,

On 20.08.26 10:29, Alexandra Winter wrote:

On 19.08.26 11:12, Hangbin Liu wrote:

quoted

quoted

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.

quoted

quoted

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