linux-kernel - Re: [syzbot] [can?] KCSAN: data-race in can_send / can

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: <CANpmjNNMT6DNEB69dTySX61dSMHc2fqMWN1sA1fcvCWSXeRugA@mail.gmail.com>
Date: Thu, 20 Mar 2025 22:19:11 +0100
From: Marco Elver <elver@...gle.com>
To: Oliver Hartkopp <socketcan@...tkopp.net>
Cc: Vincent Mailhol <mailhol.vincent@...adoo.fr>, mkl@...gutronix.de, 
	syzbot <syzbot+78ce4489b812515d5e4d@...kaller.appspotmail.com>, 
	linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com, 
	linux-can@...r.kernel.org
Subject: Re: [syzbot] [can?] KCSAN: data-race in can_send / can_send (5)

On Mon, 10 Mar 2025 at 15:36, 'Oliver Hartkopp' via syzkaller-bugs
<syzkaller-bugs@...glegroups.com> wrote:
>
> Hi Vincent, Marc,
>
> I sent a patch to be reviewed:
> https://lore.kernel.org/linux-can/20250310143353.3242-1-socketcan@hartkopp.net/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@...tkopp.net> wrote:
> >> On 10.03.25 10:29, Vincent Mailhol wrote:
> >>> On Mon. 10 Mar 2025 at 03:59, Oliver Hartkopp <socketcan@...tkopp.net> 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