| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <93d61e1c-5cdf-498e-8699-43e57cbb221b@hartkopp.net>
Date: Fri, 30 Jan 2026 10:29:37 +0100
From: Oliver Hartkopp <socketcan@...tkopp.net>
To: Marc Kleine-Budde <mkl@...gutronix.de>,
Vincent Mailhol <mailhol@...nel.org>,
Robin van der Gracht <robin@...tonic.nl>,
Oleksij Rempel <o.rempel@...gutronix.de>, kernel@...gutronix.de,
"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>
Cc: linux-can@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next v6 6/6] can: gw: use can_gw_hops instead of
sk_buff::csum_start
On 30.01.26 09:42, Oliver Hartkopp via B4 Relay wrote:
> From: Oliver Hartkopp <socketcan@...tkopp.net>
>
> As CAN skbs don't use IP checksums the skb->csum_start variable was used to
> store the can-gw CAN frame time-to-live counter together with
> skb->ip_summed set to CHECKSUM_UNNECESSARY.
>
> Remove the 'hack' using the skb->csum_start variable and move the content
> to can_skb_ext::can_gw_hops of the CAN skb extensions.
>
> The module parameter 'max_hops' has been reduced to a single byte to fit
> can_skb_ext::can_gw_hops as the maximum value to be stored is 6.
>
> Signed-off-by: Marc Kleine-Budde <mkl@...gutronix.de>
> Signed-off-by: Oliver Hartkopp <socketcan@...tkopp.net>
> ---
> drivers/net/can/vxcan.c | 7 ++++++-
> net/can/gw.c | 23 ++++++-----------------
> 2 files changed, 12 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/can/vxcan.c b/drivers/net/can/vxcan.c
> index f14c6f02b662..6e642909d6ca 100644
> --- a/drivers/net/can/vxcan.c
> +++ b/drivers/net/can/vxcan.c
> @@ -19,10 +19,11 @@
> #include <linux/can/dev.h>
> #include <linux/can/skb.h>
> #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"
>
> MODULE_DESCRIPTION("Virtual CAN Tunnel");
> @@ -37,10 +38,11 @@ struct vxcan_priv {
> 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;
>
> if (can_dropped_invalid_skb(dev, oskb))
> return NETDEV_TX_OK;
> @@ -62,11 +64,14 @@ static netdev_tx_t vxcan_xmit(struct sk_buff *oskb, struct net_device *dev)
> kfree_skb(oskb);
> goto out_unlock;
> }
>
> /* reset CAN GW hop counter */
> - skb->csum_start = 0;
> + csx = can_skb_ext_find(skb);
> + if (csx)
> + csx->can_gw_hops = 0;
> +
We are dealing with a cloned skb here, where we can not be sure that the
original skb ("oskb") was not cloned before too.
Therefore we need to take care that the skb extension can be written
without affecting potential clones of oskb.
csx = can_skb_ext_add(skb) would do a potential cow for us and also sets
can_gw_hops to zero.
Therefore the only change for vxcan.c should look like this:
if (!can_skb_ext_add(skb)) {
kfree_skb(skb);
goto out_unlock;
}
I'll wait for a respin until tomorrow.
Maybe there are other remarks for this patch set too.
Best regards,
Oliver
> skb->pkt_type = PACKET_BROADCAST;
> skb->dev = peer;
> skb->ip_summed = CHECKSUM_UNNECESSARY;
>
> len = can_skb_get_data_len(skb);
> diff --git a/net/can/gw.c b/net/can/gw.c
> index 6f158abd61aa..d83fc8242e0b 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -69,12 +69,12 @@ MODULE_ALIAS(CAN_GW_NAME);
>
> #define CGW_MIN_HOPS 1
> #define CGW_MAX_HOPS 6
> #define CGW_DEFAULT_HOPS 1
>
> -static unsigned int max_hops __read_mostly = CGW_DEFAULT_HOPS;
> -module_param(max_hops, uint, 0444);
> +static unsigned char max_hops __read_mostly = CGW_DEFAULT_HOPS;
> +module_param(max_hops, byte, 0444);
> MODULE_PARM_DESC(max_hops,
> "maximum " CAN_GW_NAME " routing hops for CAN frames "
> "(valid values: " __stringify(CGW_MIN_HOPS) "-"
> __stringify(CGW_MAX_HOPS) " hops, "
> "default: " __stringify(CGW_DEFAULT_HOPS) ")");
> @@ -478,23 +478,12 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> return;
>
> /* Do not handle CAN frames routed more than 'max_hops' times.
> * In general we should never catch this delimiter which is intended
> * to cover a misconfiguration protection (e.g. circular CAN routes).
> - *
> - * The Controller Area Network controllers only accept CAN frames with
> - * correct CRCs - which are not visible in the controller registers.
> - * According to skbuff.h documentation the csum_start element for IP
> - * checksums is undefined/unused when ip_summed == CHECKSUM_UNNECESSARY.
> - * Only CAN skbs can be processed here which already have this property.
> */
> -
> -#define cgw_hops(skb) ((skb)->csum_start)
> -
> - BUG_ON(skb->ip_summed != CHECKSUM_UNNECESSARY);
> -
> - if (cgw_hops(skb) >= max_hops) {
> + if (csx->can_gw_hops >= max_hops) {
> /* indicate deleted frames due to misconfiguration */
> gwj->deleted_frames++;
> return;
> }
>
> @@ -534,15 +523,15 @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data)
> gwj->dropped_frames++;
> return;
> }
>
> /* put the incremented hop counter in the cloned skb */
> - cgw_hops(nskb) = cgw_hops(skb) + 1;
> + ncsx->can_gw_hops = csx->can_gw_hops + 1;
>
> /* first processing of this CAN frame -> adjust to private hop limit */
> - if (gwj->limit_hops && cgw_hops(nskb) == 1)
> - cgw_hops(nskb) = max_hops - gwj->limit_hops + 1;
> + if (gwj->limit_hops && ncsx->can_gw_hops == 1)
> + ncsx->can_gw_hops = max_hops - gwj->limit_hops + 1;
>
> nskb->dev = gwj->dst.dev;
>
> /* pointer to modifiable CAN frame */
> cf = (struct canfd_frame *)nskb->data;
>
Powered by blists - more mailing lists