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

On 06.09.2021 23:17:40, Vincent MAILHOL wrote:

quoted

quoted

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?

I don't know (but I haven't checked) if the coding style doc says
anything about that.

Or is there an actual risk of doing so?

As you pointed out, this relies on the serialization of the changelink
callback by the networking stack. There's no sane way in C to track this
requirement in the networking stack, so I don't want to have any
roadblocks and/or potential bugs in the CAN code. Marking a variable as
static places it in the BSS section, right? This mean, the memory is
always "used", even if not setting the bitrate.

This is for my understanding, I will remove the static
declaration regardless of your answer.

tnx

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.

Sounds reasonable.

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

ok - BTW: I think we can remove LEDs support now, it's marked as broken
for more than 3 years.

quoted

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.

Thanks for helping taking care of the LTS kernels!

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 |