On Tue. 14 Sep. 2021 at 18:35, Vincent MAILHOL <mailhol.vincent@xxxxxxxxxx> wrote: > Hi Oliver, > > On Wed. 8 sep. 2021 at 20:41, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > > - nextdev ML > > - linux-kernel ML > > > > Hi Vincent, > > > > On 07.09.21 14:51, Vincent MAILHOL wrote: > > > On Tue. 7 Sep. 2021 at 01:03, Vincent Mailhol > > > <mailhol.vincent@xxxxxxxxxx> wrote: > > >> struct can_priv has a set of flags (can_priv::ctrlmode) which are > > >> correlated with the other fields of the structure. In > > >> can_changelink(), those flags are set first and copied to can_priv. If > > >> the function has to return early, for example due to an out of range > > >> value provided by the user, then the global configuration might become > > >> incoherent. > > >> > > >> Example: the user provides an out of range dbitrate (e.g. 20 > > >> Mbps). The command fails (-EINVAL), however the FD flag was already > > >> set resulting in a configuration where FD is on but the databittiming > > >> parameters are empty. > > > > When the ip configuration fails you get an error code. And you > > *typically* do it again to fix your wrong command line parameters. > > > > ¯\_(ツ)_/¯ > > Overall yes. I tried to think of a counterexample and the best I > could think of is if the user does: > # ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on; ip > link set can0 up > > In which case, the .ndo_open() function of the driver would be > triggered with incorrect parameters. > > > If not the attempt to set the CAN interface to 'up' will fail (as the > > last line of defense). > > Mostly correct: open_candev() will spot that the data bitrate is not set > making the .ndo_open() fails as long as the driver correctly > checks open_candev() return value. > > However, one driver fails to check the return value of open_candev(): > https://elixir.bootlin.com/linux/v5.11/source/drivers/net/can/softing/softing_fw.c#L636 > > So, for this particular driver, we can send incoherent values to the device. > > > The code with all the sanity checks is already pretty complex IMO. > > ACK. > > > I wonder if this effort is worth it. > > Well, I was thinking "this is a bug so let's fix it". But your > argument is fair. I also did not like how complex the code was > getting when trying to fix that. I guess that this bug is > acceptable. I will leave it as it is. > > Now, I am just worried about the softing driver. Actually, the softing driver is not CAN-FD capable. So there was probably no real needs to worry. > Thanks. > > > Yours sincerely, > Vincent
