netdev - Re: [PATCH net 1/2] netlink: terminate outstanding dump on socket close

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+pzZaPL5tpZ6f5crWQ3K9LGYNHdJpnTXDsGTNzZ4og4Q@mail.gmail.com>
Date: Tue, 5 Nov 2024 11:03:08 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: kuba@...nel.org, davem@...emloft.net, johannes@...solutions.net, 
	netdev@...r.kernel.org, pabeni@...hat.com, pablo@...filter.org, 
	syzkaller@...glegroups.com
Subject: Re: [PATCH net 1/2] netlink: terminate outstanding dump on socket close

On Tue, Nov 5, 2024 at 6:31 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
>
> From: Jakub Kicinski <kuba@...nel.org>
> Date: Mon,  4 Nov 2024 17:03:46 -0800
> > Netlink supports iterative dumping of data. It provides the families
> > the following ops:
> >  - start - (optional) kicks off the dumping process
> >  - dump  - actual dump helper, keeps getting called until it returns 0
> >  - done  - (optional) pairs with .start, can be used for cleanup
> > The whole process is asynchronous and the repeated calls to .dump
> > don't actually happen in a tight loop, but rather are triggered
> > in response to recvmsg() on the socket.
> >
> > This gives the user full control over the dump, but also means that
> > the user can close the socket without getting to the end of the dump.
> > To make sure .start is always paired with .done we check if there
> > is an ongoing dump before freeing the socket, and if so call .done.
> >
> > The complication is that sockets can get freed from BH and .done
> > is allowed to sleep. So we use a workqueue to defer the call, when
> > needed.
> >
> > Unfortunately this does not work correctly. What we defer is not
> > the cleanup but rather releasing a reference on the socket.
> > We have no guarantee that we own the last reference, if someone
> > else holds the socket they may release it in BH and we're back
> > to square one.
> >
> > The whole dance, however, appears to be unnecessary. Only the user
> > can interact with dumps, so we can clean up when socket is closed.
> > And close always happens in process context. Some async code may
> > still access the socket after close, queue notification skbs to it etc.
> > but no dumps can start, end or otherwise make progress.
> >
> > Delete the workqueue and flush the dump state directly from the release
> > handler. Note that further cleanup is possible in -next, for instance
> > we now always call .done before releasing the main module reference,
> > so dump doesn't have to take a reference of its own.
>
> and we can remove netns & reftracker switching for kernel socket
>
>
> >
> > Reported-by: syzbot <syzkaller@...glegroups.com>
> > Fixes: ed5d7788a934 ("netlink: Do not schedule work from sk_destruct")
>
> Do you have a link to a public report ?

I only had reports for old kernels (6.1 stable), but the repro seems
to work on current kernel.

>
> Previously syzkaller's author asked me to use a different name for
> Reported-by not to confuse their internal metrics if the report is
> generated by local syzkaller.

I definitely have upstream reports (latest tree) but no repro yet.

I can release them, but IMO this would add noise to the mailing lists,
already flooded with such reports.

Reviewed-by: Eric Dumazet <edumazet@...gle.com>