On 11.09.22 09:51, Vincent Mailhol wrote:
On Tue. 2 Aug. 2022 at 04:02, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
To simplify the testing in user space all struct canfd_frame's provided by
the CAN subsystem of the Linux kernel now have the CANFD_FDF flag set in
canfd_frame::flags.
NB: Handcrafted ETH_P_CANFD frames introduced via PF_PACKET socket might
not set this bit correctly. During the check for sufficient headroom in
PF_PACKET sk_buffs the uninitialized CAN sk_buff data structures are filled.
In the case of a CAN FD frame the CANFD_FDF flag is set accordingly.
As the CAN frame content is already zero initialized in alloc_canfd_skb()
the obsolete initialization of cf->flags in the CTU CAN FD driver has been
removed as it would overwrite the already set CANFD_FDF flag.
Signed-off-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>
---
drivers/net/can/ctucanfd/ctucanfd_base.c | 1 -
drivers/net/can/dev/skb.c | 11 +++++++++++
include/uapi/linux/can.h | 4 ++--
net/can/af_can.c | 5 +++++
4 files changed, 18 insertions(+), 3 deletions(-)
Would it be relevant to also set the flag when the skb is cloned?
diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
index 182749e858b3..24de0bb7092e 100644
--- a/include/linux/can/skb.h
+++ b/include/linux/can/skb.h
@@ -85,6 +85,7 @@ static inline void can_skb_set_owner(struct sk_buff
*skb, struct sock *sk)
static inline struct sk_buff *can_create_echo_skb(struct sk_buff *skb)
{
struct sk_buff *nskb;
+ struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
nskb = skb_clone(skb, GFP_ATOMIC);
if (unlikely(!nskb)) {
@@ -92,6 +93,9 @@ static inline struct sk_buff
*can_create_echo_skb(struct sk_buff *skb)
return NULL;
}
+ if (can_is_canfd_skb(skb))
+ cfd->flags |= CANFD_FDF;
+
can_skb_set_owner(nskb, skb->sk);
consume_skb(skb);
return nskb;
No, this should be obsolete.
The current patch set should set this bit at all creation/malloc places
of CAN FD skbs.
diff --git a/drivers/net/can/ctucanfd/ctucanfd_base.c b/drivers/net/can/ctucanfd/ctucanfd_base.c
index 3c18d028bd8c..c4026712ab7d 100644
--- a/drivers/net/can/ctucanfd/ctucanfd_base.c
+++ b/drivers/net/can/ctucanfd/ctucanfd_base.c
@@ -655,11 +655,10 @@ static void ctucan_read_rx_frame(struct ctucan_priv *priv, struct canfd_frame *c
cf->can_id = (idw & CAN_EFF_MASK) | CAN_EFF_FLAG;
else
cf->can_id = (idw >> 18) & CAN_SFF_MASK;
/* BRS, ESI, RTR Flags */
- cf->flags = 0;
if (FIELD_GET(REG_FRAME_FORMAT_W_FDF, ffw)) {
if (FIELD_GET(REG_FRAME_FORMAT_W_BRS, ffw))
cf->flags |= CANFD_BRS;
if (FIELD_GET(REG_FRAME_FORMAT_W_ESI_RSV, ffw))
cf->flags |= CANFD_ESI;
diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index b896e1ce3b47..adb413bdd734 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -242,10 +242,13 @@ struct sk_buff *alloc_canfd_skb(struct net_device *dev,
can_skb_prv(skb)->ifindex = dev->ifindex;
can_skb_prv(skb)->skbcnt = 0;
*cfd = skb_put_zero(skb, sizeof(struct canfd_frame));
+ /* set CAN FD flag by default */
+ (*cfd)->flags = CANFD_FDF;
+
return skb;
}
EXPORT_SYMBOL_GPL(alloc_canfd_skb);
struct sk_buff *alloc_can_err_skb(struct net_device *dev, struct can_frame **cf)
@@ -285,10 +288,18 @@ static bool can_skb_headroom_valid(struct net_device *dev, struct sk_buff *skb)
skb->pkt_type = PACKET_HOST;
skb_reset_mac_header(skb);
skb_reset_network_header(skb);
skb_reset_transport_header(skb);
+
+ /* set CANFD_FDF flag for CAN FD frames */
+ if (can_is_canfd_skb(skb)) {
+ struct canfd_frame *cfd;
+
+ cfd = (struct canfd_frame *)skb->data;
+ cfd->flags |= CANFD_FDF;
+ }
}
return true;
}
diff --git a/include/uapi/linux/can.h b/include/uapi/linux/can.h
index 90801ada2bbe..7b23eeeb3273 100644
--- a/include/uapi/linux/can.h
+++ b/include/uapi/linux/can.h
@@ -139,12 +139,12 @@ struct can_frame {
* The struct can_frame and struct canfd_frame intentionally share the same
* layout to be able to write CAN frame content into a CAN FD frame structure.
* When this is done the former differentiation via CAN_MTU / CANFD_MTU gets
* lost. CANFD_FDF allows programmers to mark CAN FD frames in the case of
* using struct canfd_frame for mixed CAN / CAN FD content (dual use).
- * N.B. the Kernel APIs do NOT provide mixed CAN / CAN FD content inside of
- * struct canfd_frame therefore the CANFD_FDF flag is disregarded by Linux.
+ * Since the introduction of CAN XL the CANFD_FDF flag is set in all CAN FD
+ * frame structures provided by the CAN subsystem of the Linux kernel.
*/
#define CANFD_BRS 0x01 /* bit rate switch (second bitrate for payload data) */
#define CANFD_ESI 0x02 /* error state indicator of the transmitting node */
#define CANFD_FDF 0x04 /* mark CAN FD for dual use of struct canfd_frame */
diff --git a/net/can/af_can.c b/net/can/af_can.c
index afa6c2151bc4..072a6a5c9dd1 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -203,11 +203,16 @@ int can_send(struct sk_buff *skb, int loop)
int err = -EINVAL;
if (can_is_can_skb(skb)) {
skb->protocol = htons(ETH_P_CAN);
} else if (can_is_canfd_skb(skb)) {
+ struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
+
skb->protocol = htons(ETH_P_CANFD);
+
+ /* set CAN FD flag for CAN FD frames by default */
+ cfd->flags |= CANFD_FDF;
} else {
goto inval_skb;
}
/* Make sure the CAN frame can pass the selected CAN netdevice. */
Once all the malloc and clone functions set the flag, is there still a
route that would reach the can_send() without setting the CANFD_FDF
flag? If the answer is supposedly no, maybe it is better to remove the
check (or maybe replace it with a debug message to identify missed use
cases).
The flag IS set in all skb malloc places (and therefore does not need to
be set in clone places (see my answer above).
This specific check in can_send() especially covers content that came
from userspace which is not setting this bit today.
The same is done for userspace content provided by a PF_PACKET socket in
can_skb_headroom_valid() above.
Best regards,
Oliver
