linux-kernel - Re: [PATCH] can: kvaser_pciefd: refine error prone echo_skb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <pjk5hmqqbhhbx3zq2hqc5soqrbb4ymcwicpugj7u7xs4wk3lfk@jfz4kqhagz3x>
Date: Wed, 28 May 2025 22:00:06 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Axel Forsman <axfo@...ser.com>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>, 
	Vincent Mailhol <mailhol.vincent@...adoo.fr>, Jimmy Assarsson <extja@...ser.com>, linux-can@...r.kernel.org, 
	linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org, stable@...r.kernel.org
Subject: Re: [PATCH] can: kvaser_pciefd: refine error prone echo_skb_max
 handling logic

On Wed, 28. May 13:32, Axel Forsman wrote:
> Thanks for finding and fixing this bug.
> 
> Fedor Pchelkin <pchelkin@...ras.ru> writes:
> 
> > Actually the trick with rounding up allows to calculate seq numbers
> > efficiently, avoiding a more consuming 'mod' operation used in the
> > current patch.
> 
> Indeed, that was the intention.
> 
> > So another approach to fix the problem would be to precompute the rounded
> > up value of echo_skb_max and pass it to alloc_candev() making the size of
> > the underlying echo_skb[] sufficient.
> 
> I believe that is preferable---if memory usage is a concern
> KVASER_PCIEFD_CAN_TX_MAX_COUNT could be lowered by one.
> Something like the following:
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index f6921368cd14..0071a51ce2c1 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -966,7 +966,7 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
>                 u32 status, tx_nr_packets_max;
> 
>                 netdev = alloc_candev(sizeof(struct kvaser_pciefd_can),
> -                                     KVASER_PCIEFD_CAN_TX_MAX_COUNT);
> +                                     roundup_pow_of_two(KVASER_PCIEFD_CAN_TX_MAX_COUNT));
>                 if (!netdev)
>                         return -ENOMEM;
> 
> @@ -995,7 +995,6 @@ static int kvaser_pciefd_setup_can_ctrls(struct kvaser_pciefd *pcie)
>                 can->tx_max_count = min(KVASER_PCIEFD_CAN_TX_MAX_COUNT, tx_nr_packets_max - 1);
> 
>                 can->can.clock.freq = pcie->freq;
> -               can->can.echo_skb_max = roundup_pow_of_two(can->tx_max_count);
>                 spin_lock_init(&can->lock);
> 
>                 can->can.bittiming_const = &kvaser_pciefd_bittiming_const;

Got it, thanks for review!

Setting KVASER_PCIEFD_CAN_TX_MAX_COUNT - value representing something like
the count of pending tx frames - to 17 (not even a multiple of 2) is quite
strange to me. This was probably done due to some hardware or protocol
specs though I've failed to find any evidence available in public access.

Will send v2 soon.