On Mon, 10 Mar 2025 at 15:36, 'Oliver Hartkopp' via syzkaller-bugs <syzkaller-bugs@xxxxxxxxxxxxxxxx> wrote: > > Hi Vincent, Marc, > > I sent a patch to be reviewed: > https://lore.kernel.org/linux-can/20250310143353.3242-1-socketcan@xxxxxxxxxxxx/T/#u > > I've also tested this patch without any new issues. > > Best regards, > Oliver > > On 10.03.25 10:55, Vincent Mailhol wrote: > > On Mon. 10 Mar 2025 at 18:46, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > >> On 10.03.25 10:29, Vincent Mailhol wrote: > >>> On Mon. 10 Mar 2025 at 03:59, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > > > > (...) > > > >>>> Isn't there some lock-less per-cpu safe statistic handling within netdev > >>>> we might pick for our use-case? > >>> > >>> I see two solutions. Either we use lock_sock(skb->sk) and > >>> release_sock(skb->sk) or we can change the types of > >>> can_pkg_stats->tx_frames and can_pkg_stats->tx_frames_delta from long > >>> to atomic_long_t. > >>> > >>> The atomic_long_t is the closest solution to a lock-less. But my > >>> preference goes to the lock_sock() which looks more natural in this > >>> context. And look_sock() is just a spinlock which under the hood is > >>> also an atomic, so no big penalty either. > >> > >> When we get skbs from the netdevice (and not from user space), we do not > >> have a valid sk value. It is set to zero. > >> > >> See: > >> https://elixir.bootlin.com/linux/v6.13.6/source/net/can/raw.c#L203 > >> > >> And those skbs can also be forwarded by can-gw using can_send(). > >> > >> Therefore there is no lock_sock() without a valid sk ;-) > >> > >> When 'atomic_long_t' would also fix this simple statistics handling, we > >> should use that. > > > > I see, Thanks for the explanation. Then atomic_long_t seems the best > > (and easiest). While I would prefer atomic_long_t myself, just to point out an alternative for "lossy" stats counters: could use __data_racy or data_race(..), and just accept the data race if "approximate" statistics can be lived with if the stats counting is happening from a very performance sensitive hot path. See section "Data-Racy Reads for Approximate Diagnostics" in https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/memory-model/Documentation/access-marking.txt
