On 30.07.22 15:16, Vincent Mailhol wrote:
On Sat. 30 juil. 2022 at 21:06, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote:
Existing applications are exempted from this risk: the
CAN_RAW_XL_FRAMES socket option will prevent the collision from
happening. To summarise, the collision will only occur if below
criteria are met:
* The user allocates a can_frame without zeroing it (typical use
cases are stack declaration or malloc()).
* The user activates the CAN_RAW_XL_FRAMES socket option.
* Garbage value results in can_frame::__pad & CANXL_XLF being set.
So if going in that direction, we would have to update the
documentation (header files and kernel doc) to point at this pitfall
and specify that can_frame::__pad shall always be zeroed for mix
usages.
But you would add this recommendation to a *very late point* - namely
after 14 years of SocketCAN in the mainline kernel.
If you think about the rule "never break user space" I wonder if the
risk turns out to be higher when people enable CAN_RAW_XL_FRAMES ("as
seen on Stackoverflow") and not read the kernel doc recommendations to
review/update their existing code.
In which aspect does this break the user space? It doesn't.
It does in the way that you need to zero-initialize structures now.
Enabling CAN_RAW_XL_FRAMES marks a clear transition between the 14
years old Classical CAN API and the new use case of mixed CAN(-FD) and
CAN XL frames.
You are seriously telling me that CAN_RAW_XL_FRAMES is a clear
transition and people will automatically review and rework their former
CAN/CANFD code to zero-initialize padding bytes?
There is a much higher risk that this does not happen than telling
people to check for CANXL_XLF first after reading from the socket!
For the issue to occur, developers have to incorrectly modify their
application, I do not call this a break in user space if only the
yet-to-be-created programs are impacted.
I don't know about your thresholds but I call this a break in user
space. Yet-to-be-created programs will likely copy old code.
The argument that people do not read the kernel doc apply to both
cases: some people will not initialise variables, some people will not
do the "if" checks in the right order.
To make it simple: The difference between CAN XL and (old) CAN/CANFD
handling is CANXL_XLF.
I'm really tired about your constructed case that people would be too
dumb to check for CANXL_XLF first.
This CANFD_FDF check is a new optional(!) possibility, which we do not
need to promote. This could become a power user trick for people that
are obviously "smart enough" to put if-statements in the right order.
The correct and safe check that should be documented and suggested looks
like this:
if (can.xl.flags & CANXL_XLF)
/* Handle CAN XL frame */
else if (nbytes == CANFD_MTU)
/* Handle CAN FD frame */
else if (nbytes == CAN_MTU) /* paranoid */
/* Handle Classic CAN frame */
else
/* error */
Regards,
Oliver
