linux-kernel - Re: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related error handling

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 for Android: free password hash cracker in your pocket

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

Date: Wed, 15 May 2024 15:14:59 +0300
From: Fedor Pchelkin <pchelkin@...ras.ru>
To: Robin Murphy <robin.murphy@....com>
Cc: Xiang Chen <chenxiang66@...ilicon.com>, Barry Song <21cnbao@...il.com>, 
	Christoph Hellwig <hch@....de>, Marek Szyprowski <m.szyprowski@...sung.com>, 
	iommu@...ts.linux.dev, linux-kernel@...r.kernel.org, 
	Alexey Khoroshilov <khoroshilov@...ras.ru>, lvc-project@...uxtesting.org
Subject: Re: [PATCH v2 1/4] dma-mapping: benchmark: fix up kthread-related
 error handling

Hi,

Thanks for review of the series!

Robin Murphy wrote:
> On 2024-05-04 12:47 pm, Fedor Pchelkin wrote:
> > kthread creation failure is invalidly handled inside do_map_benchmark().
> > The put_task_struct() calls on the error path are supposed to balance the
> > get_task_struct() calls which only happen after all the kthreads are
> > successfully created. Rollback using kthread_stop() for already created
> > kthreads in case of such failure.
> > 
> > In normal situation call kthread_stop_put() to gracefully stop kthreads
> > and put their task refcounts. This should be done for all started
> > kthreads.
> 
> Although strictly there were two overlapping bugs here, I'd agree that 
> it really doesn't seem worth the bother of trying to distinguish them. 
> I'm far from a kthread expert, but as best I can tell this looks like it 
> achieves a sensible final state. Modulo one nit below,
> 
> Reviewed-by: Robin Murphy <robin.murphy@....com>
> 
> > Found by Linux Verification Center (linuxtesting.org).
> > 
> > Fixes: 65789daa8087 ("dma-mapping: add benchmark support for streaming DMA APIs")
> > Suggested-by: Robin Murphy <robin.murphy@....com>
> > Signed-off-by: Fedor Pchelkin <pchelkin@...ras.ru>
> > ---
> >   kernel/dma/map_benchmark.c | 16 ++++++++++------
> >   1 file changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/kernel/dma/map_benchmark.c b/kernel/dma/map_benchmark.c
> > index 02205ab53b7e..2478957cf9f8 100644
> > --- a/kernel/dma/map_benchmark.c
> > +++ b/kernel/dma/map_benchmark.c
> > @@ -118,6 +118,8 @@ static int do_map_benchmark(struct map_benchmark_data *map)
> >   		if (IS_ERR(tsk[i])) {
> >   			pr_err("create dma_map thread failed\n");
> >   			ret = PTR_ERR(tsk[i]);
> > +			while (--i >= 0)
> > +				kthread_stop(tsk[i]);
> 
> I think this means we'd end up actually starting the threads purely to 
> get them to see the KTHREAD_SHOULD_STOP flag and exit again? Not that 
> I'm too fussed about optimising an unexpected error path, however I 
> can't help but wonder if we might only need a put_task_struct() if we 
> can safely assume that the threads have never been started at this point.

The threads have been already started to the moment by
kthread_create_on_node() but put to uninterruptible sleep: please see
kthread() function. They just have to be explicitly awakened, find that
the KTHREAD_SHOULD_STOP flag was set and perform do_exit() in order to
terminate and release all resources. The thread_fn won't be executed in
such case.

I feel there is no more convenient way for doing this differently than
calling kthread_stop(). And the comment for kthread_stop() actually implies
that it is intended to work also just after kthread creation.

Calling put_task_struct() in that place will hit WARN_ON(!tsk->exit_state).
I'd say the last call to this function should be made after (or in result
of) the do_exit().