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.
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.
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 ?
Where we define
#define CANXL_TAG 0x7F
#define CANXL_SEC 0x80
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.
(..)
You are right about the inconsistency. But it needs to be
#define CANXL_MIN_DLEN 1
#define CANXL_MAX_DLEN 2048
to fit the CAN/CANFD definitions.
ACK.
#define CANXL_MTU sizeof(struct canxl_frame)
#define CANXL_HEAD_SZ (CANXL_MTU - CANXL_MAXDLEN)
#define CANXL_MINTU (CANXL_HEAD_SZ + CANXL_MINDLEN)
I need to think twice about all that, all the different alternatives
(allow or not garbage, data as flexible member array vs.
data[CANXL_DLEN]). Now it is a bit late, so I will continue to think
about all that tomorrow.
I would suggest to not allow garbage and have skbuff::len and
canxl_frame::len in sync.
When a userspace application writes more bytes than needed by
canxl_frame::len, then that garbage is cropped to the needed size.
Cropped by whom? My point is to allow userspace to leave garbage. I am
totally OK with cropping the frames as soon as they enter the kernel.
That was exactly what I wanted to say :-D
- be very strict inside the kernel
- and crop the userspace garbage away when entering the kernel
On the other hand, the kernel shall always return cropped frames to
the userland. To summarize, skb and data len synchronisation is
optional for userland but mandatory for kernel drivers.
ACK!
Best regards,
Oliver
