Re: [PATCH v2] can: skb: add extended skb support

Oliver Hartkopp <socketcan@xxxxxxxxxxxx> · Sat, 14 May 2022 11:59:09 +0200

On 12.05.22 19:46, Oleksij Rempel wrote:
Hi Oliver,

On Thu, May 12, 2022 at 06:54:46PM +0200, Oliver Hartkopp wrote:
Hi Oleksij,

On 12.05.22 14:59, Oleksij Rempel wrote:
Add CAN specific skb extension support and add first currently needed
local_origin variable.

On the CAN stack we push same skb data in different direction depending
on the interface type:
- to the HW egress and at same time back to the stack as echo
- over virtual vcan/vxcan interfaces as egress on one side and ingress on other
    side of the vxcan tunnel.
We can't use skb->sk as marker of the origin, because not all packets
not all packets with local_origin are assigned to some socket. Some of
them are generate from the kernel, for example like J1939 control messages.
So, to properly detect flow direction is is better to store this information
as part of the SKB.

The advantage of using skb_ext is that it is options and extendable
without affecting other skb users. It can be shared between cloned skbs and
duplicated only if needed.

Signed-off-by: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
Cc: Devid Antonio Filoni <devid.filoni@xxxxxxxxxxxxxxxxxxxxx>
---
changes v2:
- migrate it to SKB_EXT

The use of SKB_EXT seems to be very costly to just store a boolean value.

What I could see from some of the other SKB_EXT users this extension (which
performs alloc & COW) is used in special circumstances.

With your suggestion this additional effort is needed for every CAN related
skb.

So at least for this use-case extending struct can_skb_priv seems to be more
efficient.

https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L44

We might get into problems with PF_PACKET sockets when extending the
can_skb_priv length beyond HH_DATA_MOD, see:

https://elixir.bootlin.com/linux/latest/source/include/linux/can/skb.h#L99

But for now I'm not sure that SKB_EXT isn't too heavy to store that single
flag.

Yes, I was thinking about potential overkill for, currently, just one
bit of storage. But here is my motivation:
CAN frameworks is currently using two ways to store metaadata (expecpt
of SKB):
1. skb->cb. This variant is not extendable and can be used only insight
    of one driver.
2. can_skb_priv as part of skb->data->head. Is potentially extendable
    but we will need to use skb_copy instead of skb_clone. Because we
    can't modify head for clone only. IMO, this will add potentially more
    overhead than SKB_EXT.

In long term, as soon as we will need to extend can specific meta
data, we will have same situation: it will be not big enough to migrate
to SKB_EXT. Maybe we need to move can_skb_priv to SKB_EXT as well?

I wonder if our current issue with the correct attribution of host 
generated CAN skbs in j1939 could be solved by additionally setting 
skb->redirected = 1 when the CAN skb is echo'ed back.

Or what about creating a single socket/sk instance for the j1939 
protocol which makes j1939 created skbs assigned to the j1939 system?

Best regards,
Oliver




   drivers/net/can/vxcan.c |  4 ++++
   include/linux/can/skb.h |  4 ++++
   include/linux/skbuff.h  |  3 +++
   net/can/Kconfig         |  1 +
   net/can/af_can.c        |  5 +++++
   net/can/raw.c           | 10 ++++++++--
   net/core/skbuff.c       |  7 +++++++
   7 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
index 577a80300514..93701a698008 100644
--- a/drivers/net/can/vxcan.c
+++ b/drivers/net/can/vxcan.c
@@ -39,6 +39,7 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
   	struct net_device *peer;
   	struct canfd_frame *cfd = (struct canfd_frame *)oskb->data;
   	struct net_device_stats *peerstats, *srcstats = &dev->stats;
+	struct can_skb_ext *can_ext;
   	struct sk_buff *skb;
   	u8 len;
@@ -66,6 +67,9 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
   	skb->pkt_type   = PACKET_BROADCAST;
   	skb->dev        = peer;
   	skb->ip_summed  = CHECKSUM_UNNECESSARY;
+	can_ext = skb_ext_add(skb, SKB_EXT_CAN);
+	if (can_ext)
+		can_ext->local_origin = false;
   	len = cfd->can_id & CAN_RTR_FLAG ? 0 : cfd->len;
   	if (netif_rx(skb) == NET_RX_SUCCESS) {
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index fdb22b00674a..401b08890d74 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -55,6 +55,10 @@ struct can_skb_priv {
   	struct can_frame cf[];
   };
+struct can_skb_ext {
+	bool local_origin;
+};
+
   static inline struct can_skb_priv *can_skb_prv(struct sk_buff *skb)
   {
   	return (struct can_skb_priv *)(skb->head);
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3270cb72e4d8..d39e70e5f7f2 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -4563,6 +4563,9 @@ enum skb_ext_id {
   #endif
   #if IS_ENABLED(CONFIG_MCTP_FLOWS)
   	SKB_EXT_MCTP,
+#endif
+#if IS_ENABLED(CONFIG_CAN)
+	SKB_EXT_CAN,
   #endif
   	SKB_EXT_NUM, /* must be last */
   };
diff --git a/net/can/Kconfig b/net/can/Kconfig
index a9ac5ffab286..eb826e3771fe 100644
--- a/net/can/Kconfig
+++ b/net/can/Kconfig
@@ -5,6 +5,7 @@
   menuconfig CAN
   	tristate "CAN bus subsystem support"
+	select SKB_EXTENSIONS
   	help
   	  Controller Area Network (CAN) is a slow (up to 1Mbit/s) serial
   	  communications protocol. Development of the CAN bus started in
diff --git a/net/can/af_can.c b/net/can/af_can.c
index 1fb49d51b25d..329c540d3ddf 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -201,6 +201,7 @@ int can_send(struct sk_buff *skb, int loop)
   	struct sk_buff *newskb = NULL;
   	struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
   	struct can_pkg_stats *pkg_stats = dev_net(skb->dev)->can.pkg_stats;
+	struct can_skb_ext *can_ext;
   	int err = -EINVAL;
   	if (skb->len == CAN_MTU) {
@@ -240,6 +241,10 @@ int can_send(struct sk_buff *skb, int loop)
   	skb_reset_network_header(skb);
   	skb_reset_transport_header(skb);
+	can_ext = skb_ext_add(skb, SKB_EXT_CAN);
+	if (can_ext)
+		can_ext->local_origin = true;
+
   	if (loop) {
   		/* local loopback of sent CAN frames */
diff --git a/net/can/raw.c b/net/can/raw.c
index b7dbb57557f3..cba18cdf017f 100644
--- a/net/can/raw.c
+++ b/net/can/raw.c
@@ -121,6 +121,7 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
   {
   	struct sock *sk = (struct sock *)data;
   	struct raw_sock *ro = raw_sk(sk);
+	struct can_skb_ext *can_ext;
   	struct sockaddr_can *addr;
   	struct sk_buff *skb;
   	unsigned int *pflags;
@@ -173,8 +174,13 @@ static void raw_rcv(struct sk_buff *oskb, void *data)
   	/* add CAN specific message flags for raw_recvmsg() */
   	pflags = raw_flags(skb);
   	*pflags = 0;
-	if (oskb->sk)
-		*pflags |= MSG_DONTROUTE;
+
+	can_ext = skb_ext_find(oskb, SKB_EXT_CAN);
+	if (can_ext) {
+		if (can_ext->local_origin)
+			*pflags |= MSG_DONTROUTE;
+	}
+
   	if (oskb->sk == sk)
   		*pflags |= MSG_CONFIRM;
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 475183f37891..5a5409ccb767 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -61,6 +61,7 @@
   #include <linux/if_vlan.h>
   #include <linux/mpls.h>
   #include <linux/kcov.h>
+#include <linux/can/skb.h>
   #include <net/protocol.h>
   #include <net/dst.h>
@@ -4338,6 +4339,9 @@ static const u8 skb_ext_type_len[] = {
   #if IS_ENABLED(CONFIG_MCTP_FLOWS)
   	[SKB_EXT_MCTP] = SKB_EXT_CHUNKSIZEOF(struct mctp_flow),
   #endif
+#if IS_ENABLED(CONFIG_CAN)
+	[SKB_EXT_CAN] = SKB_EXT_CHUNKSIZEOF(struct can_skb_ext),
+#endif
   };
   static __always_inline unsigned int skb_ext_total_length(void)
@@ -4357,6 +4361,9 @@ static __always_inline unsigned int skb_ext_total_length(void)
   #endif
   #if IS_ENABLED(CONFIG_MCTP_FLOWS)
   		skb_ext_type_len[SKB_EXT_MCTP] +
+#endif
+#if IS_ENABLED(CONFIG_CAN)
+		skb_ext_type_len[SKB_EXT_CAN] +
   #endif
   		0;
   }









