Commit 021718d2 authored by Paolo Abeni's avatar Paolo Abeni
Browse files

Merge branch 'move-can-skb-headroom-content-to-skb-extensions' 

Oliver Hartkopp says:

====================
move CAN skb headroom content to skb extensions

CAN bus related skbuffs (ETH_P_CAN/ETH_P_CANFD/ETH_P_CANXL) simply contain
CAN frame structs for CAN CC/FD/XL of skb->len length at skb->data. Those
CAN skbs do not have network/mac/transport headers nor other such
references for encapsulated protocols like ethernet/IP protocols.

To store data for CAN specific use-cases all CAN bus related skbuffs are
created with a 16 byte private skb headroom (struct can_skb_priv). Using
the skb headroom and accessing skb->head for this private data led to
several problems in the past likely due to "The struct can_skb_priv
business is highly unconventional for the networking stack." [1]

This patch set aims to remove the unconventional skb headroom usage for CAN
bus related skbuffs and use the common skb extensions instead.

[1] https://lore.kernel.org/linux-can/20260104074222.29e660ac@kernel.org/

- v1: https://patch.msgid.link/20260125201601.5018-1-socketcan@hartkopp.net
- v2: https://lore.kernel.org/linux-can/20260128-can-skb-ext-v2-0-fe64aa152c8a@pengutronix.de/
- v4: https://lore.kernel.org/netdev/20260128-can_skb_ext-v1-0-330f60fd5d7e@hartkopp.net/
- v5: https://patch.msgid.link/20260129-can_skb_ext-v5-0-21252fdc8900@hartkopp.net
- v6: https://patch.msgid.link/20260130-can_skb_ext-v6-0-8fceafab7f26@hartkopp.net
- v7: https://patch.msgid.link/20260131-can_skb_ext-v7-0-dd0f8f84a83d@hartkopp.net



Signed-off-by: default avatarOliver Hartkopp <socketcan@hartkopp.net>
====================

Link: https://patch.msgid.link/20260201-can_skb_ext-v8-0-3635d790fe8b@hartkopp.net


Signed-off-by: default avatarPaolo Abeni <pabeni@redhat.com>
parents 0cbcc0fd 3ffecc14

MAINTAINERS

+1 −0
Original line number Diff line number Diff line
@@ -5634,6 +5634,7 @@ F: Documentation/networking/iso15765-2.rst 
F:	include/linux/can/can-ml.h

F:	include/linux/can/core.h

F:	include/linux/can/skb.h

F:	include/net/can.h

F:	include/net/netns/can.h

F:	include/uapi/linux/can.h

F:	include/uapi/linux/can/bcm.h

drivers/net/can/dev/skb.c

+77 −46
Original line number Diff line number Diff line
@@ -6,6 +6,7 @@ 


#include <linux/can/dev.h>

#include <linux/module.h>

#include <net/can.h>



#define MOD_DESC "CAN device driver interface"
@@ -48,6 +49,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, 
		     unsigned int idx, unsigned int frame_len)

{

	struct can_priv *priv = netdev_priv(dev);

	struct can_skb_ext *csx;



	if (idx >= priv->echo_skb_max) {

		netdev_err(dev, "%s: BUG! Trying to access can_priv::echo_skb out of bounds (%u/max %u)\n",
@@ -74,7 +76,9 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, 
		skb->dev = dev;



		/* save frame_len to reuse it when transmission is completed */

		can_skb_prv(skb)->frame_len = frame_len;

		csx = can_skb_ext_find(skb);

		if (csx)

			csx->can_framelen = frame_len;



		if (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP)

			skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
@@ -111,7 +115,7 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, 
		 * length is supported on both CAN and CANFD frames.

		 */

		struct sk_buff *skb = priv->echo_skb[idx];

		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);

		struct can_skb_ext *csx;



		if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)

			skb_tstamp_tx(skb, skb_hwtstamps(skb));
@@ -119,8 +123,13 @@ __can_get_echo_skb(struct net_device *dev, unsigned int idx, 
		/* get the real payload length for netdev statistics */

		*len_ptr = can_skb_get_data_len(skb);



		if (frame_len_ptr)

			*frame_len_ptr = can_skb_priv->frame_len;

		if (frame_len_ptr) {

			csx = can_skb_ext_find(skb);

			if (csx)

				*frame_len_ptr = csx->can_framelen;

			else

				*frame_len_ptr = 0;

		}



		priv->echo_skb[idx] = NULL;
@@ -180,10 +189,15 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx, 


	if (priv->echo_skb[idx]) {

		struct sk_buff *skb = priv->echo_skb[idx];

		struct can_skb_priv *can_skb_priv = can_skb_prv(skb);

		struct can_skb_ext *csx;



		if (frame_len_ptr)

			*frame_len_ptr = can_skb_priv->frame_len;

		if (frame_len_ptr) {

			csx = can_skb_ext_find(skb);

			if (csx)

				*frame_len_ptr = csx->can_framelen;

			else

				*frame_len_ptr = 0;

		}



		dev_kfree_skb_any(skb);

		priv->echo_skb[idx] = NULL;
@@ -192,38 +206,39 @@ void can_free_echo_skb(struct net_device *dev, unsigned int idx, 
EXPORT_SYMBOL_GPL(can_free_echo_skb);



/* fill common values for CAN sk_buffs */

static void init_can_skb_reserve(struct sk_buff *skb)

static void init_can_skb(struct sk_buff *skb)

{

	skb->pkt_type = PACKET_BROADCAST;

	skb->ip_summed = CHECKSUM_UNNECESSARY;



	skb_reset_mac_header(skb);

	skb_reset_network_header(skb);

	skb_reset_transport_header(skb);



	can_skb_reserve(skb);

	can_skb_prv(skb)->skbcnt = 0;

}



struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)

{

	struct sk_buff *skb;

	struct can_skb_ext *csx;



	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +

			       sizeof(struct can_frame));

	if (unlikely(!skb)) {

		*cf = NULL;

	skb = netdev_alloc_skb(dev, sizeof(struct can_frame));

	if (unlikely(!skb))

		goto out_error_cc;



		return NULL;

	csx = can_skb_ext_add(skb);

	if (!csx) {

		kfree_skb(skb);

		goto out_error_cc;

	}



	skb->protocol = htons(ETH_P_CAN);

	init_can_skb_reserve(skb);

	can_skb_prv(skb)->ifindex = dev->ifindex;

	init_can_skb(skb);

	csx->can_iif = dev->ifindex;



	*cf = skb_put_zero(skb, sizeof(struct can_frame));



	return skb;



out_error_cc:

	*cf = NULL;



	return NULL;

}

EXPORT_SYMBOL_GPL(alloc_can_skb);
@@ -231,18 +246,21 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, 
				struct canfd_frame **cfd)

{

	struct sk_buff *skb;

	struct can_skb_ext *csx;



	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +

			       sizeof(struct canfd_frame));

	if (unlikely(!skb)) {

		*cfd = NULL;

	skb = netdev_alloc_skb(dev, sizeof(struct canfd_frame));

	if (unlikely(!skb))

		goto out_error_fd;



		return NULL;

	csx = can_skb_ext_add(skb);

	if (!csx) {

		kfree_skb(skb);

		goto out_error_fd;

	}



	skb->protocol = htons(ETH_P_CANFD);

	init_can_skb_reserve(skb);

	can_skb_prv(skb)->ifindex = dev->ifindex;

	init_can_skb(skb);

	csx->can_iif = dev->ifindex;



	*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
@@ -250,6 +268,11 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev, 
	(*cfd)->flags = CANFD_FDF;



	return skb;



out_error_fd:

	*cfd = NULL;



	return NULL;

}

EXPORT_SYMBOL_GPL(alloc_canfd_skb);
@@ -258,18 +281,24 @@ struct sk_buff *alloc_canxl_skb(struct net_device *dev, 
				unsigned int data_len)

{

	struct sk_buff *skb;

	struct can_skb_ext *csx;



	if (data_len < CANXL_MIN_DLEN || data_len > CANXL_MAX_DLEN)

		goto out_error;

		goto out_error_xl;



	skb = netdev_alloc_skb(dev, sizeof(struct can_skb_priv) +

			       CANXL_HDR_SIZE + data_len);

	skb = netdev_alloc_skb(dev, CANXL_HDR_SIZE + data_len);

	if (unlikely(!skb))

		goto out_error;

		goto out_error_xl;



	csx = can_skb_ext_add(skb);

	if (!csx) {

		kfree_skb(skb);

		goto out_error_xl;

	}



	skb->protocol = htons(ETH_P_CANXL);

	init_can_skb_reserve(skb);

	can_skb_prv(skb)->ifindex = dev->ifindex;

	init_can_skb(skb);

	csx->can_iif = dev->ifindex;



	*cxl = skb_put_zero(skb, CANXL_HDR_SIZE + data_len);
@@ -279,7 +308,7 @@ struct sk_buff *alloc_canxl_skb(struct net_device *dev, 


	return skb;



out_error:

out_error_xl:

	*cxl = NULL;



	return NULL;
@@ -302,18 +331,20 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf) 
EXPORT_SYMBOL_GPL(alloc_can_err_skb);



/* Check for outgoing skbs that have not been created by the CAN subsystem */

static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)

static bool can_skb_init_valid(struct net_device *dev, struct sk_buff *skb)

{

	/* af_packet creates a headroom of HH_DATA_MOD bytes which is fine */

	if (WARN_ON_ONCE(skb_headroom(skb) < sizeof(struct can_skb_priv)))

		return false;

	struct can_skb_ext *csx = can_skb_ext_find(skb);



	/* af_packet does not apply CAN skb specific settings */

	if (skb->ip_summed == CHECKSUM_NONE) {

		/* init headroom */

		can_skb_prv(skb)->ifindex = dev->ifindex;

		can_skb_prv(skb)->skbcnt = 0;

	if (skb->ip_summed == CHECKSUM_NONE || !csx) {

		/* init CAN skb content */

		if (!csx) {

			csx = can_skb_ext_add(skb);

			if (!csx)

				return false;

		}



		csx->can_iif = dev->ifindex;

		skb->ip_summed = CHECKSUM_UNNECESSARY;



		/* perform proper loopback on capable devices */
@@ -361,7 +392,7 @@ bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb) 
		goto inval_skb;

	}



	if (!can_skb_headroom_valid(dev, skb))

	if (!can_skb_init_valid(dev, skb))

		goto inval_skb;



	return false;

drivers/net/can/vxcan.c

+14 −1
Original line number Diff line number Diff line
@@ -21,6 +21,7 @@ 
#include <linux/can/vxcan.h>

#include <linux/can/can-ml.h>

#include <linux/slab.h>

#include <net/can.h>

#include <net/rtnetlink.h>



#define DRV_NAME "vxcan"
@@ -39,6 +40,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) 
	struct vxcan_priv *priv = netdev_priv(dev);

	struct net_device *peer;

	struct net_device_stats *peerstats, *srcstats = &dev->stats;

	struct can_skb_ext *csx;

	struct sk_buff *skb;

	unsigned int len;
@@ -63,8 +65,19 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev) 
		goto out_unlock;

	}



	/* the cloned skb points to the skb extension of the already cloned

	 * oskb with an increased refcount. skb_ext_add() creates a copy to

	 * separate the skb extension data which is needed to start with a

	 * fresh can_gw_hops counter in the other namespace.

	 */

	csx = skb_ext_add(skb, SKB_EXT_CAN);

	if (!csx) {

		kfree_skb(skb);

		goto out_unlock;

	}



	/* reset CAN GW hop counter */

	skb->csum_start = 0;

	csx->can_gw_hops = 0;

	skb->pkt_type   = PACKET_BROADCAST;

	skb->dev        = peer;

	skb->ip_summed  = CHECKSUM_UNNECESSARY;

include/linux/can/core.h

+1 −0
Original line number Diff line number Diff line
@@ -58,6 +58,7 @@ extern void can_rx_unregister(struct net *net, struct net_device *dev, 
			      void *data);



extern int can_send(struct sk_buff *skb, int loop);

void can_set_skb_uid(struct sk_buff *skb);

void can_sock_destruct(struct sock *sk);



#endif /* !_CAN_CORE_H */

include/linux/can/skb.h

+11 −27
Original line number Diff line number Diff line
@@ -14,6 +14,7 @@ 
#include <linux/types.h>

#include <linux/skbuff.h>

#include <linux/can.h>

#include <net/can.h>

#include <net/sock.h>



void can_flush_echo_skb(struct net_device *dev);
@@ -37,37 +38,20 @@ struct sk_buff *alloc_can_err_skb(struct net_device *dev, 
				  struct can_frame **cf);

bool can_dropped_invalid_skb(struct net_device *dev, struct sk_buff *skb);



/*

 * The struct can_skb_priv is used to transport additional information along

 * with the stored struct can(fd)_frame that can not be contained in existing

 * struct sk_buff elements.

 * N.B. that this information must not be modified in cloned CAN sk_buffs.

 * To modify the CAN frame content or the struct can_skb_priv content

 * skb_copy() needs to be used instead of skb_clone().

 */



/**

 * struct can_skb_priv - private additional data inside CAN sk_buffs

 * @ifindex:	ifindex of the first interface the CAN frame appeared on

 * @skbcnt:	atomic counter to have an unique id together with skb pointer

 * @frame_len:	length of CAN frame in data link layer

 * @cf:		align to the following CAN frame at skb->data

 */

struct can_skb_priv {

	int ifindex;

	int skbcnt;

	unsigned int frame_len;

	struct can_frame cf[];

};



static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)

static inline struct can_skb_ext *can_skb_ext_add(struct sk_buff *skb)

{

	return (struct can_skb_priv *)(skb->head);

	struct can_skb_ext *csx = skb_ext_add(skb, SKB_EXT_CAN);



	/* skb_ext_add() returns uninitialized space */

	if (csx)

		csx->can_gw_hops = 0;



	return csx;

}



static inline void can_skb_reserve(struct sk_buff *skb)

static inline struct can_skb_ext *can_skb_ext_find(struct sk_buff *skb)

{

	skb_reserve(skb, sizeof(struct can_skb_priv));

	return skb_ext_find(skb, SKB_EXT_CAN);

}



static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)