Re: [RESEND PATCH v2] can: netlink: prevent incoherent can configuration in case of early return
From: Vincent MAILHOL
Date: Mon Sep 06 2021 - 10:18:01 EST
On Mon. 6 Sep 2021 at 17:18, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 03.09.2021 16:17:04, Vincent Mailhol 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.
> > * 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
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:
> > 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.
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> > Resending because I got no answers on:
> > https://lore.kernel.org/linux-can/20210823024750.702542-1-mailhol.vincent@xxxxxxxxxx/T/#u
> > (I guess everyone bas busy with the upcoming merge window)
> Busy yes, but not with the merge window :)
> > 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.
> 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 |