On Thu. 14 Jul. 2022 at 05:02, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> On 13.07.22 03:07, Vincent Mailhol wrote:
> > On Wed. 13 Jul. 2022 at 04:24, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
> > Well, if we document than can_frame::__pad shall be zero for mix
> > usages (i.e. comments in struct can_frame and updated kernel doc),
> > then we would have done our due diligence. From that point, if people
> > ignore the documentation *and* do not follow best practices for safety
> > application development, I wouldn't cry.
>
> Right. As long as we did not enable CAN_XL we do not need to take care.
> And introducing the restriction to set __pad to zero together with the
> new CAN_XL option seems legit.
ACK. I draw the line between the already existing applications and the
yet to be written ones. If we are safe for the existing ones, then I
think we are good to go.
> > If we absolutely want to prevent struct can_frame to be interpreted as
> > a canxl_frame due to some stack garbage, we can add one
> > padding/reserved field like that:
> >
> > struct canxl_frame {
> > canid_t prio; /* 11 bit priority for arbitration (canid_t) */
> > __u8 sdt; /* SDU (service data unit) type */
> > __u8 flags; /* additional flags for CAN XL */
> > __u16 len; /* frame payload length in byte */
> > __u32 af; /* acceptance field */
> > __u32 __res; /* reserved field. Shall be zero */
> > __u8 data[] __attribute__((aligned(8)));
> > };
> >
> > This way, the minimum transfer unit of CANXL is 17 bytes (16 for
> > header and 1 for data) which is exactly one byte more than can_frame
> > (and we get back the 8 bytes alignment \o/)
> >
> > This would only leave the risk of having some garbage in
> > canfd_frame::flags, e.g. if user does:
> >
> > struct canfd_frame cfd; /* declared on the stack and not initialized */
> > cfd.flags |= <some_flags> /* use |= instead of = */
> >
> > But this is already risky for plain CAN-FD.
>
> Hm, this brings me to an even more weird idea:
>
> struct canxl_frame {
> canid_t prio; /* 11 bit priority for arbitration (canid_t) */
> __u8 sec:1, /* SEC bit */
> xltag:7; /* all bits set */
> __u8 sdt; /* SDU (service data unit) type */
> __u16 len; /* frame payload length in byte */
> __u32 af; /* acceptance field */
> __u8 data[CANXL_MAX_DLEN];
> };
>
> We could set the first byte (former len element) to a value of 0x7F and
> define the SEC bit as 0x80.
Beware! You just stepped in the realm of undefined behaviours! The
order of the bitfields is implementation specific...
Ref: ISO/IEC 9899:1999 section 6.7.2.1 "Structure and union
specifiers" paragraph 10:
| The order of allocation of bit-fields within a unit (high-order to
| low-order or low-order to high-order) is implementation-defined.
Example, on my x86_64 machine, this code:
/*********** begin **********/
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
union foo {
struct {
uint8_t sec:1, /* SEC bit */
xltag:7; /* all bits set */
};
uint8_t raw;
};
int main()
{
union foo foo;
foo.sec = 1;
printf("foo.raw: 0x%x\n", foo.raw);
return EXIT_SUCCESS;
}
/*********** end ************/
will return: "foo.raw: 0x1" instead of the expected 0x80.
The correct declaration is:
/*********** begin **********/
#include <asm/byteorder.h>
struct canxl_frame {
canid_t prio; /* 11 bit priority for arbitration (canid_t) */
#if defined(__LITTLE_ENDIAN_BITFIELD)
__u8 xltag:7, /* all bits set */
sec:1; /* SEC bit */
#else /* __BIG_ENDIAN_BITFIELD */
__u8 sec:1, /* SEC bit */
xltag:7; /* all bits set */
#endif
__u8 sdt; /* SDU (service data unit) type */
__u16 len; /* frame payload length in byte */
__u32 af; /* acceptance field */
__u8 data[CANXL_MAX_DLEN];
};
/*********** end ************/
And this starts to be a really convoluted solution.
> This would mean that we use the former length element to indicate the
> CAN XL frame as no CAN/CANFD frame can have a length of 127 and above.
>
> Not sure if it makes sense to define some bits as depicted above, or if
> we should define a __u8 xlsec ?
If we follow your idea, use __u8 xlsec in order to avoid undefined behaviours.
> Where we define
>
> #define CANXL_TAG 0x7F
Here, you "burn" all the flags for future usage.
FYI, this doesn't have to be 0x7F. It could be any of the length
values not allowed by CAN-FD, namely (in decimal): 9-11, 13-15, 17-19,
21-23, 25-31, 33-47, 49-63
> #define CANXL_SEC 0x80
I did not get why CANXL_XLF isn't needed anymore.
> which has to be set in the xlsec element then.
>
> With this move we get rid of any flags problems (we only need the SEC
> bit anyway) and define a clear 'escape value' in the former length element.
If I try to bounce on your idea, I will propose:
/*********** begin **********/
struct canxl_frame {
canid_t prio; /* 11 bit priority for arbitration (canid_t) */
__u8 flags; /* additional flags for CAN XL. MSB must be set */
__u8 sdt; /* SDU (service data unit) type */
__u16 len; /* frame payload length in bytes */
__u32 af; /* acceptance field */
__u8 data[];
};
#define CANXL_XLF 0x01 /* mark CAN XL for dual use of struct canfd_frame */
#define CANXL_SEC 0x02 /* Simple Extended Content (security/segmentation) */
#define CANXL_DISCRIMINATOR 0x80; /* Mandatory to distinguish between
CAN(-FD) and XL frames */
/*********** end ************/
This has no undefined behaviour and we still have five flags (0x04 to
0x40) for future use.
Yours sincerely,
Vincent Mailhol
