Revert "can: raw: instantly reject unsupported CAN frames"
Subsystems (2)
Oliver Hartkopp
maintainerPATCH 1/3 Revert "can: raw: instantly reject unsupported CAN frames"
This reverts commit 1a620a723853a0f49703c317d52dc6b9602cbaa8
and its follow-up fixes for the introduced dependency issues.
commit 1a620a723853 ("can: raw: instantly reject unsupported CAN frames") commit cb2dc6d2869a ("can: Kconfig: select CAN driver infrastructure by default") commit 6abd4577bccc ("can: fix build dependency")
commit 5a5aff6338c0 ("can: fix build dependency")
The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems a better approach has been developed which makes use of CAN specific ml_priv data which is accessible from both sides.
--- drivers/net/can/Kconfig | 7 +++-- drivers/net/can/Makefile | 2 +- drivers/net/can/dev/Makefile | 5 ++-- include/linux/can/dev.h | 7 ----- net/can/raw.c | 54 ++++++------------------------------ 5 files changed, 17 insertions(+), 58 deletions(-)
drivers/net/can/Kconfig +5 -2
@@ -1,9 +1,9 @@ # SPDX-License-Identifier: GPL-2.0-only menuconfig CAN_DEV - bool "CAN Device Drivers" + tristate "CAN Device Drivers" default y depends on CAN help Controller Area Network (CAN) is serial communications protocol up to 1Mbit/s for its original release (now known as Classical CAN) and up
@@ -15,11 +15,14 @@ menuconfig CAN_DEV This section contains all the CAN(-FD) device drivers including the virtual ones. If you own such devices or plan to use the virtual CAN interfaces to develop applications, say Y here. -if CAN_DEV && CAN + To compile as a module, choose M here: the module will be called + can-dev. + +if CAN_DEV config CAN_VCAN tristate "Virtual Local CAN Interface (vcan)" help Similar to the network loopback devices, vcan offers a
drivers/net/can/Makefile +1 -1
@@ -5,11 +5,11 @@ obj-$(CONFIG_CAN_VCAN) += vcan.o obj-$(CONFIG_CAN_VXCAN) += vxcan.o obj-$(CONFIG_CAN_SLCAN) += slcan/ -obj-$(CONFIG_CAN_DEV) += dev/ +obj-y += dev/ obj-y += esd/ obj-y += rcar/ obj-y += rockchip/ obj-y += spi/ obj-y += usb/
drivers/net/can/dev/Makefile +3 -2
@@ -1,10 +1,11 @@ # SPDX-License-Identifier: GPL-2.0 -obj-$(CONFIG_CAN) += can-dev.o +obj-$(CONFIG_CAN_DEV) += can-dev.o + +can-dev-y += skb.o -can-dev-$(CONFIG_CAN_DEV) += skb.o can-dev-$(CONFIG_CAN_CALC_BITTIMING) += calc_bittiming.o can-dev-$(CONFIG_CAN_NETLINK) += bittiming.o can-dev-$(CONFIG_CAN_NETLINK) += dev.o can-dev-$(CONFIG_CAN_NETLINK) += length.o can-dev-$(CONFIG_CAN_NETLINK) += netlink.o
include/linux/can/dev.h +0 -7
@@ -109,18 +109,11 @@ struct net_device *alloc_candev_mqs(int sizeof_priv, unsigned int echo_skb_max,#define alloc_candev_mq (sizeof_priv, echo_skb_max, count) \ alloc_candev_mqs(sizeof_priv, echo_skb_max, count, count)void free_candev (struct net_device *dev);/* a candev safe wrapper around netdev_priv */ -#if IS_ENABLED (CONFIG_CAN_NETLINK )struct can_priv *safe_candev_priv (struct net_device *dev); -#else -static inline struct can_priv *safe_candev_priv (struct net_device *dev) -{ -return NULL ; -} -#endif int open_candev (struct net_device *dev);void close_candev (struct net_device *dev);void can_set_default_mtu (struct net_device *dev); int __must_check can_set_static_ctrlmode(struct net_device *dev,
net/can/raw.c +8 -46
81 lines not on this page · raw message
Signature · 1 line
2.47.3
Oliver Hartkopp
maintainerPATCH 2/3 can: propagate CAN device capabilities via ml_priv
Commit 1a620a723853 ("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.
The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific ml_priv data which is accessible from both sides.
With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
--- drivers/net/can/dev/dev.c | 24 ++++++++++++++++++++++++ drivers/net/can/dev/netlink.c | 1 + drivers/net/can/vcan.c | 15 +++++++++++++++ drivers/net/can/vxcan.c | 15 +++++++++++++++ include/linux/can/can-ml.h | 25 +++++++++++++++++++++++++ include/linux/can/dev.h | 1 + 6 files changed, 81 insertions(+)
drivers/net/can/dev/dev.c +24 -0
@@ -373,10 +373,33 @@ void can_set_default_mtu(struct net_device *dev) dev->min_mtu =CAN_MTU ; dev->max_mtu =CAN_MTU ; } } +void can_set_cap_info (struct net_device *dev) +{ +struct can_priv *priv =netdev_priv (dev); +u32 can_cap =CAN_CAP_CC ; + +if (can_dev_in_xl_only_mode (priv)) { +/* XL only mode => no CC/FD capability */ + can_cap =CAN_CAP_XL ; + }else { +if (priv->ctrlmode &CAN_CTRLMODE_FD ) + can_cap |=CAN_CAP_FD ; + +if (priv->ctrlmode &CAN_CTRLMODE_XL ) + can_cap |=CAN_CAP_XL ; + } + +if (priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_RESTRICTED )) + can_cap |=CAN_CAP_RO ; + +can_set_cap (dev, can_cap); +} +/* helper to define static CAN controller features at device creation time */ int can_set_static_ctrlmode (struct net_device *dev,u32 static_mode) {struct can_priv *priv =netdev_priv (dev);
@@ -388,10 +411,11 @@ int can_set_static_ctrlmode(struct net_device *dev, u32 static_mode) } priv->ctrlmode = static_mode;/* override MTU which was set by default in can_setup()? */ can_set_default_mtu (dev); +can_set_cap_info (dev);return 0 ; }EXPORT_SYMBOL_GPL (can_set_static_ctrlmode);
drivers/net/can/dev/netlink.c +1 -0
@@ -375,10 +375,11 @@ static int can_ctrlmode_changelink(struct net_device *dev,memset (&priv->xl.tdc,0 ,sizeof (priv->xl.tdc));memset (&priv->xl.pwm,0 ,sizeof (priv->xl.pwm)); }can_set_default_mtu (dev); +can_set_cap_info (dev);return 0 ; } static int can_tdc_changelink(struct data_bittiming_params *dbt_params,
drivers/net/can/vcan.c +15 -0
@@ -128,10 +128,23 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)consume_skb (skb); }return NETDEV_TX_OK ; } +static void vcan_set_cap_info (struct net_device *dev) +{ +u32 can_cap =CAN_CAP_CC ; + +if (dev->mtu >CAN_MTU ) + can_cap |=CAN_CAP_FD ; + +if (dev->mtu >=CANXL_MIN_MTU ) + can_cap |=CAN_CAP_XL ; + +can_set_cap (dev, can_cap); +} +static int vcan_change_mtu (struct net_device *dev,int new_mtu) {/* Do not allow changing the MTU while running */ if (dev->flags &IFF_UP )return -EBUSY ;
@@ -139,10 +152,11 @@ static int vcan_change_mtu(struct net_device *dev, int new_mtu)if (new_mtu !=CAN_MTU && new_mtu !=CANFD_MTU && !can_is_canxl_dev_mtu (new_mtu))return -EINVAL ;WRITE_ONCE (dev->mtu, new_mtu); +vcan_set_cap_info (dev);return 0 ; }static const struct net_device_ops vcan_netdev_ops = { .ndo_start_xmit = vcan_tx,
@@ -160,10 +174,11 @@ static void vcan_setup(struct net_device *dev) dev->hard_header_len =0 ; dev->addr_len =0 ; dev->tx_queue_len =0 ; dev->flags =IFF_NOARP ;can_set_ml_priv (dev,netdev_priv (dev)); +vcan_set_cap_info (dev);/* set flags according to driver capabilities */ if (echo) dev->flags |=IFF_ECHO ;
drivers/net/can/vxcan.c +15 -0
@@ -123,10 +123,23 @@ static int vxcan_get_iflink(const struct net_device *dev)rcu_read_unlock ();return iflink; } +static void vxcan_set_cap_info (struct net_device *dev) +{ +u32 can_cap =CAN_CAP_CC ; + +if (dev->mtu >CAN_MTU ) + can_cap |=CAN_CAP_FD ; + +if (dev->mtu >=CANXL_MIN_MTU ) + can_cap |=CAN_CAP_XL ; + +can_set_cap (dev, can_cap); +} +static int vxcan_change_mtu (struct net_device *dev,int new_mtu) {/* Do not allow changing the MTU while running */ if (dev->flags &IFF_UP )return -EBUSY ;
@@ -134,10 +147,11 @@ static int vxcan_change_mtu(struct net_device *dev, int new_mtu)if (new_mtu !=CAN_MTU && new_mtu !=CANFD_MTU && !can_is_canxl_dev_mtu (new_mtu))return -EINVAL ;WRITE_ONCE (dev->mtu, new_mtu); +vxcan_set_cap_info (dev);return 0 ; }static const struct net_device_ops vxcan_netdev_ops = { .ndo_open = vxcan_open,
@@ -165,10 +179,11 @@ static void vxcan_setup(struct net_device *dev) dev->ethtool_ops = &vxcan_ethtool_ops; dev->needs_free_netdev = true; can_ml =netdev_priv (dev) +ALIGN (sizeof (struct vxcan_priv ),NETDEV_ALIGN );can_set_ml_priv (dev, can_ml); +vxcan_set_cap_info (dev); }/* forward declaration for rtnl_create_link() */ static struct rtnl_link_ops vxcan_link_ops;
include/linux/can/can-ml.h +25 -0
@@ -44,10 +44,16 @@#include <linux/can.h> #include <linux/list.h> #include <linux/netdevice.h> +/* exposed CAN device capabilities for network layer */ +#define CAN_CAP_CC 0x1U/* CAN CC aka Classical CAN */ +#define CAN_CAP_FD 0x2U/* CAN FD */ +#define CAN_CAP_XL 0x4U/* CAN XL */ +#define CAN_CAP_RO 0x8U/* read-only mode (LISTEN/RESTRICTED) */ +#define CAN_SFF_RCV_ARRAY_SZ (1 << CAN_SFF_ID_BITS)#define CAN_EFF_RCV_HASH_BITS 10#define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)enum {RX_ERR ,RX_ALL ,RX_FIL ,RX_INV ,RX_MAX };
@@ -62,10 +68,11 @@ struct can_dev_rcv_lists {struct can_ml_priv {struct can_dev_rcv_lists dev_rcv_lists;#ifdef CAN_J1939 struct j1939_priv *j1939_priv;#endif +u32 can_cap; };static inline struct can_ml_priv *can_get_ml_priv (struct net_device *dev) {return netdev_get_ml_priv (dev,ML_PRIV_CAN );
@@ -75,6 +82,24 @@ static inline void can_set_ml_priv(struct net_device *dev, struct can_ml_priv *ml_priv) {netdev_set_ml_priv (dev, ml_priv,ML_PRIV_CAN ); } +static inline bool can_cap_enabled (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (!can_ml) +return true; + +return (can_ml->can_cap & cap); +} + +static inline void can_set_cap (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (can_ml) + can_ml->can_cap = cap; +} +#endif /* CAN_ML_H */
include/linux/can/dev.h +1 -0
@@ -114,10 +114,11 @@ void free_candev(struct net_device *dev);struct can_priv *safe_candev_priv (struct net_device *dev);int open_candev (struct net_device *dev);void close_candev (struct net_device *dev);void can_set_default_mtu (struct net_device *dev); +void can_set_cap_info (struct net_device *dev);int __must_check can_set_static_ctrlmode (struct net_device *dev,u32 static_mode);int can_hwtstamp_get (struct net_device *netdev,struct kernel_hwtstamp_config *cfg); int can_hwtstamp_set(struct net_device *netdev,
Signature · 1 line
2.47.3
Oliver Hartkopp
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
Hi Marc/Vincent,
Quoted 9 lines · Oliver Hartkopp
Commit
1a620a723853("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific
ml_privdata which is accessible from both sides.
Quoted 4 lines
With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")
Are you fine with this patch set?
I've tested it and it works as expected.
IMO the two patches after the revert should have no problems to be accepted as they restore the functionality we already had in 6.19-rc1.
Best regards,
Oliver
Quoted 257 lines
Cc Marc Kleine-Budde <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Vincent Mailhol <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>--- drivers/net/can/dev/dev.c | 24 ++++++++++++++++++++++++ drivers/net/can/dev/netlink.c | 1 + drivers/net/can/vcan.c | 15 +++++++++++++++ drivers/net/can/vxcan.c | 15 +++++++++++++++ include/linux/can/can-ml.h | 25 +++++++++++++++++++++++++ include/linux/can/dev.h | 1 + 6 files changed, 81 insertions(+)drivers/net/can/dev/dev.c +24
-0@@ -373,10 +373,33 @@ void can_set_default_mtu(struct net_device *dev) dev->min_mtu =CAN_MTU ; dev->max_mtu =CAN_MTU ; } } +void can_set_cap_info (struct net_device *dev) +{ +struct can_priv *priv =netdev_priv (dev); +u32 can_cap =CAN_CAP_CC ; + +if (can_dev_in_xl_only_mode (priv)) { +/* XL only mode => no CC/FD capability */ + can_cap =CAN_CAP_XL ; + }else { +if (priv->ctrlmode &CAN_CTRLMODE_FD ) + can_cap |=CAN_CAP_FD ; + +if (priv->ctrlmode &CAN_CTRLMODE_XL ) + can_cap |=CAN_CAP_XL ; + } + +if (priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_RESTRICTED )) + can_cap |=CAN_CAP_RO ; + +can_set_cap (dev, can_cap); +} +/* helper to define static CAN controller features at device creation time */ int can_set_static_ctrlmode (struct net_device *dev,u32 static_mode) {struct can_priv *priv =netdev_priv (dev);@@ -388,10 +411,11 @@ int can_set_static_ctrlmode(struct net_device *dev, u32 static_mode) } priv->ctrlmode = static_mode;/* override MTU which was set by default in can_setup()? */ can_set_default_mtu (dev); +can_set_cap_info (dev);return 0 ; }EXPORT_SYMBOL_GPL (can_set_static_ctrlmode);drivers/net/can/dev/netlink.c +1
-0@@ -375,10 +375,11 @@ static int can_ctrlmode_changelink(struct net_device *dev,memset (&priv->xl.tdc,0 ,sizeof (priv->xl.tdc));memset (&priv->xl.pwm,0 ,sizeof (priv->xl.pwm)); }can_set_default_mtu (dev); +can_set_cap_info (dev);return 0 ; } static int can_tdc_changelink(struct data_bittiming_params *dbt_params,drivers/net/can/vcan.c +15
-0@@ -128,10 +128,23 @@ static netdev_tx_t vcan_tx(struct sk_buff *skb, struct net_device *dev)consume_skb (skb); }return NETDEV_TX_OK ; } +static void vcan_set_cap_info (struct net_device *dev) +{ +u32 can_cap =CAN_CAP_CC ; + +if (dev->mtu >CAN_MTU ) + can_cap |=CAN_CAP_FD ; + +if (dev->mtu >=CANXL_MIN_MTU ) + can_cap |=CAN_CAP_XL ; + +can_set_cap (dev, can_cap); +} +static int vcan_change_mtu (struct net_device *dev,int new_mtu) {/* Do not allow changing the MTU while running */ if (dev->flags &IFF_UP )return -EBUSY ;@@ -139,10 +152,11 @@ static int vcan_change_mtu(struct net_device *dev, int new_mtu)if (new_mtu !=CAN_MTU && new_mtu !=CANFD_MTU && !can_is_canxl_dev_mtu (new_mtu))return -EINVAL ;WRITE_ONCE (dev->mtu, new_mtu); +vcan_set_cap_info (dev);return 0 ; }static const struct net_device_ops vcan_netdev_ops = { .ndo_start_xmit = vcan_tx,@@ -160,10 +174,11 @@ static void vcan_setup(struct net_device *dev) dev->hard_header_len =0 ; dev->addr_len =0 ; dev->tx_queue_len =0 ; dev->flags =IFF_NOARP ;can_set_ml_priv (dev,netdev_priv (dev)); +vcan_set_cap_info (dev);/* set flags according to driver capabilities */ if (echo) dev->flags |=IFF_ECHO ;drivers/net/can/vxcan.c +15
-0@@ -123,10 +123,23 @@ static int vxcan_get_iflink(const struct net_device *dev)rcu_read_unlock ();return iflink; } +static void vxcan_set_cap_info (struct net_device *dev) +{ +u32 can_cap =CAN_CAP_CC ; + +if (dev->mtu >CAN_MTU ) + can_cap |=CAN_CAP_FD ; + +if (dev->mtu >=CANXL_MIN_MTU ) + can_cap |=CAN_CAP_XL ; + +can_set_cap (dev, can_cap); +} +static int vxcan_change_mtu (struct net_device *dev,int new_mtu) {/* Do not allow changing the MTU while running */ if (dev->flags &IFF_UP )return -EBUSY ;@@ -134,10 +147,11 @@ static int vxcan_change_mtu(struct net_device *dev, int new_mtu)if (new_mtu !=CAN_MTU && new_mtu !=CANFD_MTU && !can_is_canxl_dev_mtu (new_mtu))return -EINVAL ;WRITE_ONCE (dev->mtu, new_mtu); +vxcan_set_cap_info (dev);return 0 ; }static const struct net_device_ops vxcan_netdev_ops = { .ndo_open = vxcan_open,@@ -165,10 +179,11 @@ static void vxcan_setup(struct net_device *dev) dev->ethtool_ops = &vxcan_ethtool_ops; dev->needs_free_netdev = true; can_ml =netdev_priv (dev) +ALIGN (sizeof (struct vxcan_priv ),NETDEV_ALIGN );can_set_ml_priv (dev, can_ml); +vxcan_set_cap_info (dev); }/* forward declaration for rtnl_create_link() */ static struct rtnl_link_ops vxcan_link_ops;include/linux/can/can-ml.h +25
-0@@ -44,10 +44,16 @@#include <linux/can.h> #include <linux/list.h> #include <linux/netdevice.h> +/* exposed CAN device capabilities for network layer */ +#define CAN_CAP_CC 0x1U/* CAN CC aka Classical CAN */ +#define CAN_CAP_FD 0x2U/* CAN FD */ +#define CAN_CAP_XL 0x4U/* CAN XL */ +#define CAN_CAP_RO 0x8U/* read-only mode (LISTEN/RESTRICTED) */ +#define CAN_SFF_RCV_ARRAY_SZ (1 << CAN_SFF_ID_BITS)#define CAN_EFF_RCV_HASH_BITS 10#define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)enum {RX_ERR ,RX_ALL ,RX_FIL ,RX_INV ,RX_MAX };@@ -62,10 +68,11 @@ struct can_dev_rcv_lists {struct can_ml_priv {struct can_dev_rcv_lists dev_rcv_lists;#ifdef CAN_J1939 struct j1939_priv *j1939_priv;#endif +u32 can_cap; };static inline struct can_ml_priv *can_get_ml_priv (struct net_device *dev) {return netdev_get_ml_priv (dev,ML_PRIV_CAN );@@ -75,6 +82,24 @@ static inline void can_set_ml_priv(struct net_device *dev, struct can_ml_priv *ml_priv) {netdev_set_ml_priv (dev, ml_priv,ML_PRIV_CAN ); } +static inline bool can_cap_enabled (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (!can_ml) +return true; + +return (can_ml->can_cap & cap); +} + +static inline void can_set_cap (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (can_ml) + can_ml->can_cap = cap; +} +#endif /* CAN_ML_H */ include/linux/can/dev.h +1
-0@@ -114,10 +114,11 @@ void free_candev(struct net_device *dev);struct can_priv *safe_candev_priv (struct net_device *dev);int open_candev (struct net_device *dev);void close_candev (struct net_device *dev);void can_set_default_mtu (struct net_device *dev); +void can_set_cap_info (struct net_device *dev);int __must_check can_set_static_ctrlmode (struct net_device *dev,u32 static_mode);int can_hwtstamp_get (struct net_device *netdev,struct kernel_hwtstamp_config *cfg); int can_hwtstamp_set(struct net_device *netdev,
Marc Kleine-Budde
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
Quoted 10 lines · Oliver Hartkopp
Quoted 9 lines · Oliver Hartkopp
Commit
1a620a723853("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific
ml_privdata which is accessible from both sides.
Quoted 7 lines
Quoted 4 lines
With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")Are you fine with this patch set?
I've tested it and it works as expected.
It seems to be OK as an interim solution, that fixes current mess.
But the main problem is, that we don't have the "struct can_priv can" in the netdev_priv of virtual devices. That should be fixed sooner or later.
Quoted 2 lines
IMO the two patches after the revert should have no problems to be accepted as they restore the functionality we already had in 6.19-rc1.
Yes, I think so.
regards,
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Oliver Hartkopp
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
Quoted 16 lines · Marc Kleine-Budde
Quoted 15 lines · Oliver Hartkopp
Quoted 13 lines · Oliver Hartkopp
Commit
1a620a723853("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific
ml_privdata which is accessible from both sides.With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")
Quoted 8 lines
Quoted 2 lines
Are you fine with this patch set?
I've tested it and it works as expected.It seems to be OK as an interim solution, that fixes current mess.
But the main problem is, that we don't have the "struct
can_privcan" in thenetdev_privof virtual devices. That should be fixed sooner or later.
I don't think so. In fact the virtual CAN interfaces vcan/vxcan don't need any CAN driver infrastructure to do their work. So why require to add lots of code for hardware-less setups?
When you really want to play with a virtual CAN driver that supports the entire struct can_priv mechanic and netlink configuration, Vincents new dummy_can driver does an excellent work.
Maybe we should improve the v[x]can drivers to reduce code duplications in the future.
Quoted 4 lines
Quoted 2 lines
IMO the two patches after the revert should have no problems to be accepted as they restore the functionality we already had in 6.19-rc1.
Yes, I think so.
Thanks. The three patches are candidates for Jakubs "Current release - fix to a fix:" section in his PR description ;-)
Best regards,
Oliver
Marc Kleine-Budde
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
In reply to Oliver HartkoppCan you please add a cover letter to this series.
more comments inline...
Quoted 31 lines · Oliver Hartkopp
Commit
1a620a723853("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific
ml_privdata which is accessible from both sides.With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")Cc Marc Kleine-Budde <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Vincent Mailhol <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>--- drivers/net/can/dev/dev.c | 24 ++++++++++++++++++++++++ drivers/net/can/dev/netlink.c | 1 + drivers/net/can/vcan.c | 15 +++++++++++++++ drivers/net/can/vxcan.c | 15 +++++++++++++++ include/linux/can/can-ml.h | 25 +++++++++++++++++++++++++ include/linux/can/dev.h | 1 + 6 files changed, 81 insertions(+)drivers/net/can/dev/dev.c +0
-0
drivers/net/can/dev/dev.c void can_set_default_mtu(struct net_device *dev) · line 381
Quoted 9 lines
drivers/net/can/dev/dev.c +4
-0@@ -373,10 +373,33 @@ void can_set_default_mtu(struct net_device *dev) dev->min_mtu =CAN_MTU ; dev->max_mtu =CAN_MTU ; } } +void can_set_cap_info (struct net_device *dev) +{ +struct can_priv *priv =netdev_priv (dev); +u32 can_cap =CAN_CAP_CC ;
I personally I would move the CAN_CAP_CC into the else branch. YMMV.
Quoted 19 lines
drivers/net/can/dev/dev.c +19
-0+ +if (can_dev_in_xl_only_mode (priv)) { +/* XL only mode => no CC/FD capability */ + can_cap =CAN_CAP_XL ; + }else { +if (priv->ctrlmode &CAN_CTRLMODE_FD ) + can_cap |=CAN_CAP_FD ; + +if (priv->ctrlmode &CAN_CTRLMODE_XL ) + can_cap |=CAN_CAP_XL ; + } + +if (priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_RESTRICTED )) + can_cap |=CAN_CAP_RO ; + +can_set_cap (dev, can_cap); +} +
[...]
Quoted 5 lines
include/linux/can/can-ml.h +0
-0
include/linux/can/can-ml.h line 53
Quoted 10 lines
include/linux/can/can-ml.h +5
-0@@ -44,10 +44,16 @@#include <linux/can.h> #include <linux/list.h> #include <linux/netdevice.h> +/* exposed CAN device capabilities for network layer */ +#define CAN_CAP_CC 0x1U/* CAN CC aka Classical CAN */ +#define CAN_CAP_FD 0x2U/* CAN FD */ +#define CAN_CAP_XL 0x4U/* CAN XL */ +#define CAN_CAP_RO 0x8U/* read-only mode (LISTEN/RESTRICTED) */
Can we use BIT() here?
Quoted 20 lines
include/linux/can/can-ml.h +2
-0+#define CAN_SFF_RCV_ARRAY_SZ (1 << CAN_SFF_ID_BITS)#define CAN_EFF_RCV_HASH_BITS 10#define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)enum {RX_ERR ,RX_ALL ,RX_FIL ,RX_INV ,RX_MAX };@@ -62,10 +68,11 @@ struct can_dev_rcv_lists {struct can_ml_priv {struct can_dev_rcv_lists dev_rcv_lists;#ifdef CAN_J1939 struct j1939_priv *j1939_priv;#endif +u32 can_cap; };static inline struct can_ml_priv *can_get_ml_priv (struct net_device *dev) {return netdev_get_ml_priv (dev,ML_PRIV_CAN );@@ -75,6 +82,24 @@ static inline void can_set_ml_priv(struct net_device *dev, struct can_ml_priv *ml_priv)
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 92
Quoted 10 lines
include/linux/can/can-ml.h +6
-0{netdev_set_ml_priv (dev, ml_priv,ML_PRIV_CAN ); } +static inline bool can_cap_enabled (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (!can_ml) +return true;
Why is the capability enabled, if the device doesn't have a CAN ml_priv?
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 101
Quoted 9 lines
include/linux/can/can-ml.h +9
-0+ +return (can_ml->can_cap & cap); +} + +static inline void can_set_cap (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (can_ml)
IMHO it's a bug if can_ml is NULL, it should not be ignored silently. Maybe just de-ref the NULL pointer?
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 105
Quoted 4 lines
include/linux/can/can-ml.h +3
-0+ can_ml->can_cap = cap; +} +#endif /* CAN_ML_H */
Thanks,
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Oliver Hartkopp
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
Quoted 1 line · Marc Kleine-Budde
Can you please add a cover letter to this series.
Ok.
Quoted 40 lines
more comments inline...
Quoted 36 lines · Oliver Hartkopp
Commit
1a620a723853("can: raw: instantly reject unsupported CAN frames") caused a sequence of dependency and linker fixes.The entire problem was caused by the requirement that a new network layer feature needed to know about the protocol capabilities of the CAN devices. Instead of accessing CAN device internal data structures which caused the dependency problems this patch introduces capability information into the CAN specific
ml_privdata which is accessible from both sides.With this change the CAN network layer can check the required features and the decoupling of the driver layer and network layer is restored.
Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")Cc Marc Kleine-Budde <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Vincent Mailhol <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>--- drivers/net/can/dev/dev.c | 24 ++++++++++++++++++++++++ drivers/net/can/dev/netlink.c | 1 + drivers/net/can/vcan.c | 15 +++++++++++++++ drivers/net/can/vxcan.c | 15 +++++++++++++++ include/linux/can/can-ml.h | 25 +++++++++++++++++++++++++ include/linux/can/dev.h | 1 + 6 files changed, 81 insertions(+)drivers/net/can/dev/dev.c +0
-0@@ -373,10 +373,33 @@ void can_set_default_mtu(struct net_device *dev) dev->min_mtu =CAN_MTU ; dev->max_mtu =CAN_MTU ; } }
Quoted 7 lines
drivers/net/can/dev/dev.c void can_set_default_mtu(struct net_device *dev) · line 381
Quoted 4 lines
drivers/net/can/dev/dev.c +4
-0+void can_set_cap_info (struct net_device *dev) +{ +struct can_priv *priv =netdev_priv (dev); +u32 can_cap =CAN_CAP_CC ;I personally I would move the
CAN_CAP_CCinto the else branch. YMMV.
Ok.
Quoted 32 lines
Quoted 19 lines
drivers/net/can/dev/dev.c +19
-0+ +if (can_dev_in_xl_only_mode (priv)) { +/* XL only mode => no CC/FD capability */ + can_cap =CAN_CAP_XL ; + }else { +if (priv->ctrlmode &CAN_CTRLMODE_FD ) + can_cap |=CAN_CAP_FD ; + +if (priv->ctrlmode &CAN_CTRLMODE_XL ) + can_cap |=CAN_CAP_XL ; + } + +if (priv->ctrlmode & (CAN_CTRLMODE_LISTENONLY | +CAN_CTRLMODE_RESTRICTED )) + can_cap |=CAN_CAP_RO ; + +can_set_cap (dev, can_cap); +} +[...]
Quoted 10 lines
include/linux/can/can-ml.h +0
-0@@ -44,10 +44,16 @@#include <linux/can.h> #include <linux/list.h> #include <linux/netdevice.h>
Quoted 8 lines
include/linux/can/can-ml.h line 53
Quoted 5 lines
include/linux/can/can-ml.h +5
-0+/* exposed CAN device capabilities for network layer */ +#define CAN_CAP_CC 0x1U/* CAN CC aka Classical CAN */ +#define CAN_CAP_FD 0x2U/* CAN FD */ +#define CAN_CAP_XL 0x4U/* CAN XL */ +#define CAN_CAP_RO 0x8U/* read-only mode (LISTEN/RESTRICTED) */ Can we use
BIT()here?
Yes. Good idea.
Quoted 24 lines
Quoted 24 lines
include/linux/can/can-ml.h +2
-0+#define CAN_SFF_RCV_ARRAY_SZ (1 << CAN_SFF_ID_BITS)#define CAN_EFF_RCV_HASH_BITS 10#define CAN_EFF_RCV_ARRAY_SZ (1 << CAN_EFF_RCV_HASH_BITS)enum {RX_ERR ,RX_ALL ,RX_FIL ,RX_INV ,RX_MAX };@@ -62,10 +68,11 @@ struct can_dev_rcv_lists {struct can_ml_priv {struct can_dev_rcv_lists dev_rcv_lists;#ifdef CAN_J1939 struct j1939_priv *j1939_priv;#endif +u32 can_cap; };static inline struct can_ml_priv *can_get_ml_priv (struct net_device *dev) {return netdev_get_ml_priv (dev,ML_PRIV_CAN );@@ -75,6 +82,24 @@ static inline void can_set_ml_priv(struct net_device *dev, struct can_ml_priv *ml_priv) {netdev_set_ml_priv (dev, ml_priv,ML_PRIV_CAN ); }
Quoted 8 lines
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 92
Quoted 6 lines
include/linux/can/can-ml.h +6
-0+static inline bool can_cap_enabled (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (!can_ml) +return true;Why is the capability enabled, if the device doesn't have a CAN
ml_priv?
I have been thought about virtual CAN interfaces first but they also have a can_ml.
It's just a double-check if we really have a CAN interface here. But returning false would make more sense then. Will change.
Quoted 4 lines
Quoted 4 lines
include/linux/can/can-ml.h +4
-0+ +return (can_ml->can_cap & cap); +} +
Quoted 8 lines
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 101
Quoted 5 lines
include/linux/can/can-ml.h +5
-0+static inline void can_set_cap (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (can_ml)IMHO it's a bug if
can_mlis NULL, it should not be ignored silently. Maybe just de-ref the NULL pointer?
Right the callers have a valid can_ml before calling can_set_cap(). Will remove this check.
Best regards,
Oliver
Quoted 9 lines
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 105
Quoted 5 lines
include/linux/can/can-ml.h +3
-0+ can_ml->can_cap = cap; +} +#endif /* CAN_ML_H */ Thanks,
Marc
Marc Kleine-Budde
maintainerRe: [PATCH 2/3] can: propagate CAN device capabilities via ml_priv
Quoted 9 lines · Oliver Hartkopp
Quoted 8 lines
include/linux/can/can-ml.h static inline void can_set_ml_priv(struct net_device *dev, · line 92
Quoted 6 lines
include/linux/can/can-ml.h +6
-0+static inline bool can_cap_enabled (struct net_device *dev,u32 cap) +{ +struct can_ml_priv *can_ml =can_get_ml_priv (dev); + +if (!can_ml) +return true;Why is the capability enabled, if the device doesn't have a CAN
ml_priv?
Quoted 5 lines
I have been thought about virtual CAN interfaces first but they also have a
can_ml.It's just a double-check if we really have a CAN interface here. But returning false would make more sense then. Will change.
...or do we want to just de-ref here, too?
Marc
Signature · 4 lines
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung Nürnberg | Phone: +49-5121-206917-129 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Oliver Hartkopp
maintainerPATCH 3/3 can: raw: instantly reject disabled CAN frames
In reply to Oliver HartkoppFor real CAN interfaces the CAN_CTRLMODE_FD and CAN_CTRLMODE_XL control modes indicate whether an interface can handle those CAN FD/XL frames.
In the case a CAN XL interface is configured in CANXL-only mode with disabled error-signalling neither CAN CC nor CAN FD frames can be sent.
The checks are now performed on CAN_RAW sockets to give an instant feedback to the user when writing unsupported CAN frames to the interface or when the CAN interface is in read-only mode.
--- net/can/raw.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
net/can/raw.c +14 -9
@@ -48,11 +48,11 @@#include <linux/socket.h> #include <linux/if_arp.h> #include <linux/skbuff.h> #include <linux/can.h> #include <linux/can/core.h> -#include <linux/can/dev.h> /* for can_is_canxl_dev_mtu() */ +#include <linux/can/can-ml.h> /* for can_cap_enabled() */ #include <linux/can/skb.h> #include <linux/can/raw.h> #include <net/sock.h> #include <net/net_namespace.h>
@@ -890,24 +890,25 @@ static void raw_put_canxl_vcid(struct raw_sock *ro, struct sk_buff *skb) cxl->prio &=CANXL_PRIO_MASK ; cxl->prio |= ro->tx_vcid_shifted; } } -static unsigned int raw_check_txframe (struct raw_sock *ro,struct sk_buff *skb,int mtu) +static unsigned int raw_check_txframe (struct raw_sock *ro,struct sk_buff *skb, +struct net_device *dev) { -/* Classical CAN -> no checks for flags and device capabilities */ -if (can_is_can_skb (skb)) +/* Classical CAN */ +if (can_is_can_skb (skb) &&can_cap_enabled (dev,CAN_CAP_CC ))return CAN_MTU ; -/* CAN FD -> needs to be enabled and a CAN FD or CAN XL device */ +/* CAN FD */ if (ro->fd_frames &&can_is_canfd_skb (skb) && - (mtu ==CANFD_MTU ||can_is_canxl_dev_mtu (mtu))) +can_cap_enabled (dev,CAN_CAP_FD ))return CANFD_MTU ; -/* CAN XL -> needs to be enabled and a CAN XL device */ +/* CAN XL */ if (ro->xl_frames &&can_is_canxl_skb (skb) && -can_is_canxl_dev_mtu (mtu)) +can_cap_enabled (dev,CAN_CAP_XL ))return CANXL_MTU ;return 0 ; }
@@ -942,10 +943,14 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) dev =dev_get_by_index (sock_net (sk), ifindex);if (!dev)return -ENXIO ; +/* no sending on a CAN device in read-only mode */ +if (can_cap_enabled (dev,CAN_CAP_RO )) +return -EACCES ; + skb =sock_alloc_send_skb (sk, size +sizeof (struct can_skb_priv ), msg->msg_flags &MSG_DONTWAIT , &err);if (!skb) goto put_dev;
@@ -959,11 +964,11 @@ static int raw_sendmsg(struct socket *sock, struct msghdr *msg, size_t size) goto free_skb; err = -EINVAL ;/* check for valid CAN (CC/FD/XL) frame content */ - txmtu =raw_check_txframe (ro, skb,READ_ONCE (dev->mtu)); + txmtu =raw_check_txframe (ro, skb, dev);if (!txmtu) goto free_skb;/* only CANXL: clear/forward/set VCID value */ if (txmtu ==CANXL_MTU )
Signature · 1 line
2.47.3
Marc Kleine-Budde
maintainerRe: [PATCH 3/3] can: raw: instantly reject disabled CAN frames
Quoted 24 lines · Oliver Hartkopp
For real CAN interfaces the
CAN_CTRLMODE_FDandCAN_CTRLMODE_XLcontrol modes indicate whether an interface can handle those CAN FD/XL frames.In the case a CAN XL interface is configured in CANXL-only mode with disabled error-signalling neither CAN CC nor CAN FD frames can be sent.
The checks are now performed on
CAN_RAWsockets to give an instant feedback to the user when writing unsupported CAN frames to the interface or when the CAN interface is in read-only mode.Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")Cc Marc Kleine-Budde <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Vincent Mailhol <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>--- net/can/raw.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)net/can/raw.c +0
-0
net/can/raw.c line 53
Quoted 7 lines
net/can/raw.c +1
-1@@ -48,11 +48,11 @@#include <linux/socket.h> #include <linux/if_arp.h> #include <linux/skbuff.h> #include <linux/can.h> #include <linux/can/core.h> -#include <linux/can/dev.h> /* for can_is_canxl_dev_mtu() */ +#include <linux/can/can-ml.h> /* for can_cap_enabled() */
I think we can remove the comment, please keep the list sorted.
otherwise LGTM, thanks
Marc
--
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Oliver Hartkopp
maintainerRe: [PATCH 3/3] can: raw: instantly reject disabled CAN frames
Quoted 20 lines · Marc Kleine-Budde
Quoted 19 lines · Oliver Hartkopp
For real CAN interfaces the
CAN_CTRLMODE_FDandCAN_CTRLMODE_XLcontrol modes indicate whether an interface can handle those CAN FD/XL frames.In the case a CAN XL interface is configured in CANXL-only mode with disabled error-signalling neither CAN CC nor CAN FD frames can be sent.
The checks are now performed on
CAN_RAWsockets to give an instant feedback to the user when writing unsupported CAN frames to the interface or when the CAN interface is in read-only mode.Fixes 1a620a723853 ("can: raw: instantly reject unsupported CAN frames")Cc Marc Kleine-Budde <[email protected]>Cc Arnd Bergmann <[email protected]>Cc Vincent Mailhol <[email protected]>Signed-off-by Oliver Hartkopp <[email protected]>--- net/can/raw.c | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-)
Quoted 14 lines
net/can/raw.c line 53
Quoted 12 lines
net/can/raw.c +1
-1@@ -48,11 +48,11 @@#include <linux/socket.h> #include <linux/if_arp.h> #include <linux/skbuff.h> #include <linux/can.h> #include <linux/can/core.h> -#include <linux/can/dev.h> /* for can_is_canxl_dev_mtu() */ +#include <linux/can/can-ml.h> /* for can_cap_enabled() */ I think we can remove the comment, please keep the list sorted.
Ok. Will do.
Quoted 3 lines
otherwise LGTM, thanks
Marc
Thanks for review!
Oliver
Quoted 6 lines
--
Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de |Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |