Re: [RESEND PATCH v2] can: netlink: prevent incoherent can configuration in case of early return

On Mon. 6 Sep 2021 at 17:18, Marc Kleine-Budde [off-list ref] wrote:

On 03.09.2021 16:17:04, Vincent Mailhol wrote:

quoted

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.

* Illustration of above example *

| $ ip link set can0 type can bitrate 500000 dbitrate 20000000 fd on
| RTNETLINK answers: Invalid argument
| $ ip --details link show can0
| 1: can0: <NOARP,ECHO> mtu 72 qdisc noop state DOWN mode DEFAULT group default qlen 10
|     link/can  promiscuity 0 minmtu 0 maxmtu 0
|     can <FD> state STOPPED restart-ms 0
           ^^ FD flag is set without any of the databittiming parameters...
|       bitrate 500000 sample-point 0.875
|       tq 12 prop-seg 69 phase-seg1 70 phase-seg2 20 sjw 1
|       ES582.1/ES584.1: tseg1 2..256 tseg2 2..128 sjw 1..128 brp 1..512 brp-inc 1
|       ES582.1/ES584.1: dtseg1 2..32 dtseg2 1..16 dsjw 1..8 dbrp 1..32 dbrp-inc 1
|       clock 80000000 numtxqueues 1 numrxqueues 1 gso_max_size 65536 gso_max_segs 65535

To prevent this from happening, we do a local copy of can_priv, work
on it, an copy it at the very end of the function (i.e. only if all
previous checks succeeded).

I don't like the optimization of using a static priv. If it's too big to
be allocated on the stack, allocate it on the heap, i.e. using
kmemdup()/kfree().

The static declaration is only an issue of coding style, correct?
Or is there an actual risk of doing so?
This is for my understanding, I will remove the static
declaration regardless of your answer.

On my x86_64 machine, sizeof(priv) is 448 and if I declare priv on the stack:
| $ objdump -d drivers/net/can/dev/netlink.o | ./scripts/checkstack.pl
| 0x00000000000002100 can_changelink []:            1200

So I will allocate it on the heap.

N.B. In above figures CONFIG_CAN_LEDS is *off* because that driver
was tagged as broken in:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=30f3b42147ba6f29bc95c1bba34468740762d91b

quoted

Once this done, there is no more need to have a temporary variable for
a specific parameter. As such, the bittiming and data bittiming (bt
and dbt) are directly written to the temporary priv variable.

Finally, function can_calc_tdco() was retrieving can_priv from the
net_device and directly modifying it. We changed the prototype so that
it instead writes its changes into our temporary priv variable.

Is it possible to split this into a separate patch, so that the part
without the tdco can be backported more easily to older kernels not
having tdco? The patch fixing the tdco would be the 2nd patch...

ACK. I will send a v3 with that split.

quoted

Signed-off-by: Vincent Mailhol <redacted>
---
Resending because I got no answers on:
https://lore.kernel.org/linux-can/20210823024750.702542-1-mailhol.vincent@wanadoo.fr/T/#u (local)
(I guess everyone bas busy with the upcoming merge window)

Busy yes, but not with the merge window :)

quoted

I am not sure whether or not this needs a "Fixes" tag. Just in case,
there it is:

Fixes: 9859ccd2c8be ("can: introduce the data bitrate configuration for CAN FD")

...if it's possible to split this patch into 2 parts, add individual
fixes tags to them.

ACK.

regards,
Marc

--
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |