LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mm: dmapool: Check the dma pool name
@ 2018-05-30 11:28 Baolin Wang
  2018-05-30 12:01 ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Baolin Wang @ 2018-05-30 11:28 UTC (permalink / raw)
  To: linux-mm; +Cc: linux-kernel, arnd, broonie, baolin.wang

It will be crash if we pass one NULL name when creating one dma pool,
so we should check the passing name when copy it to dma pool.

Moreover this patch replaces kmalloc_node() with kzalloc_node() to make
sure the name array of dma pool is initialized in case the passing name
is NULL.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
 mm/dmapool.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/dmapool.c b/mm/dmapool.c
index 4d90a64..349f13d 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -155,11 +155,12 @@ struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	else if ((boundary < size) || (boundary & (boundary - 1)))
 		return NULL;
 
-	retval = kmalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
+	retval = kzalloc_node(sizeof(*retval), GFP_KERNEL, dev_to_node(dev));
 	if (!retval)
 		return retval;
 
-	strlcpy(retval->name, name, sizeof(retval->name));
+	if (name)
+		strlcpy(retval->name, name, sizeof(retval->name));
 
 	retval->dev = dev;
 
-- 
1.7.9.5

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: dmapool: Check the dma pool name
  2018-05-30 11:28 [PATCH] mm: dmapool: Check the dma pool name Baolin Wang
@ 2018-05-30 12:01 ` Matthew Wilcox
  2018-05-30 12:14   ` Baolin Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2018-05-30 12:01 UTC (permalink / raw)
  To: Baolin Wang; +Cc: linux-mm, linux-kernel, arnd, broonie

On Wed, May 30, 2018 at 07:28:43PM +0800, Baolin Wang wrote:
> It will be crash if we pass one NULL name when creating one dma pool,
> so we should check the passing name when copy it to dma pool.

NAK.  Crashing is the appropriate thing to do.  Fix the caller to not
pass NULL.

If you permit NULL to be passed then you're inviting crashes or just
bad reporting later when pool->name is printed.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: dmapool: Check the dma pool name
  2018-05-30 12:01 ` Matthew Wilcox
@ 2018-05-30 12:14   ` Baolin Wang
  2018-05-30 15:13     ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Baolin Wang @ 2018-05-30 12:14 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: linux-mm, LKML, Arnd Bergmann, Mark Brown

On 30 May 2018 at 20:01, Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, May 30, 2018 at 07:28:43PM +0800, Baolin Wang wrote:
>> It will be crash if we pass one NULL name when creating one dma pool,
>> so we should check the passing name when copy it to dma pool.
>
> NAK.  Crashing is the appropriate thing to do.  Fix the caller to not
> pass NULL.
>
> If you permit NULL to be passed then you're inviting crashes or just
> bad reporting later when pool->name is printed.

I think it just prints one NULL pool name. Sometimes the device
doesn't care the dma pool names, so I think we can make code more
solid to valid the passing parameters like other code does.
Or can we add check to return NULL when the passing name is NULL
instead of crashing the kernel? Thanks.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: dmapool: Check the dma pool name
  2018-05-30 12:14   ` Baolin Wang
@ 2018-05-30 15:13     ` Matthew Wilcox
  2018-05-30 15:41       ` Matthew Wilcox
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2018-05-30 15:13 UTC (permalink / raw)
  To: Baolin Wang; +Cc: linux-mm, LKML, Arnd Bergmann, Mark Brown

On Wed, May 30, 2018 at 08:14:09PM +0800, Baolin Wang wrote:
> On 30 May 2018 at 20:01, Matthew Wilcox <willy@infradead.org> wrote:
> > On Wed, May 30, 2018 at 07:28:43PM +0800, Baolin Wang wrote:
> >> It will be crash if we pass one NULL name when creating one dma pool,
> >> so we should check the passing name when copy it to dma pool.
> >
> > NAK.  Crashing is the appropriate thing to do.  Fix the caller to not
> > pass NULL.
> >
> > If you permit NULL to be passed then you're inviting crashes or just
> > bad reporting later when pool->name is printed.
> 
> I think it just prints one NULL pool name. Sometimes the device
> doesn't care the dma pool names, so I think we can make code more
> solid to valid the passing parameters like other code does.
> Or can we add check to return NULL when the passing name is NULL
> instead of crashing the kernel? Thanks.

No.  Fix your driver.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: dmapool: Check the dma pool name
  2018-05-30 15:13     ` Matthew Wilcox
@ 2018-05-30 15:41       ` Matthew Wilcox
  2018-05-31  2:34         ` Baolin Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Wilcox @ 2018-05-30 15:41 UTC (permalink / raw)
  To: Baolin Wang; +Cc: linux-mm, LKML, Arnd Bergmann, Mark Brown

On Wed, May 30, 2018 at 08:13:27AM -0700, Matthew Wilcox wrote:
> On Wed, May 30, 2018 at 08:14:09PM +0800, Baolin Wang wrote:
> > On 30 May 2018 at 20:01, Matthew Wilcox <willy@infradead.org> wrote:
> > > On Wed, May 30, 2018 at 07:28:43PM +0800, Baolin Wang wrote:
> > >> It will be crash if we pass one NULL name when creating one dma pool,
> > >> so we should check the passing name when copy it to dma pool.
> > >
> > > NAK.  Crashing is the appropriate thing to do.  Fix the caller to not
> > > pass NULL.
> > >
> > > If you permit NULL to be passed then you're inviting crashes or just
> > > bad reporting later when pool->name is printed.
> > 
> > I think it just prints one NULL pool name. Sometimes the device
> > doesn't care the dma pool names, so I think we can make code more
> > solid to valid the passing parameters like other code does.
> > Or can we add check to return NULL when the passing name is NULL
> > instead of crashing the kernel? Thanks.
> 
> No.  Fix your driver.

Let me elaborate on this.  Kernel code is supposed to be "reasonable".
That means we don't check every argument to every function for sanity,
unless it's going to cause trouble later.  Crashing immediately with
a bogus argument is fine; you can see the problem and fix it immediately.
Returning NULL with a bad argument is actually worse; you won't know why
the function returned NULL (maybe we're out of memory?) and you'll have
a more complex debugging experience.

Sometimes it makes sense to accept a NULL pointer and do something
reasonable, like kfree().  In this case, we can eliminate checks in all
the callers.  But we don't, in general, put sanity checks on arguments
without a good reason.

Your reasons aren't good.  "The driver doesn't care" -- well, just pass
the driver's name, then.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: dmapool: Check the dma pool name
  2018-05-30 15:41       ` Matthew Wilcox
@ 2018-05-31  2:34         ` Baolin Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Baolin Wang @ 2018-05-31  2:34 UTC (permalink / raw)
  To: Matthew Wilcox; +Cc: linux-mm, LKML, Arnd Bergmann, Mark Brown

On 30 May 2018 at 23:41, Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, May 30, 2018 at 08:13:27AM -0700, Matthew Wilcox wrote:
>> On Wed, May 30, 2018 at 08:14:09PM +0800, Baolin Wang wrote:
>> > On 30 May 2018 at 20:01, Matthew Wilcox <willy@infradead.org> wrote:
>> > > On Wed, May 30, 2018 at 07:28:43PM +0800, Baolin Wang wrote:
>> > >> It will be crash if we pass one NULL name when creating one dma pool,
>> > >> so we should check the passing name when copy it to dma pool.
>> > >
>> > > NAK.  Crashing is the appropriate thing to do.  Fix the caller to not
>> > > pass NULL.
>> > >
>> > > If you permit NULL to be passed then you're inviting crashes or just
>> > > bad reporting later when pool->name is printed.
>> >
>> > I think it just prints one NULL pool name. Sometimes the device
>> > doesn't care the dma pool names, so I think we can make code more
>> > solid to valid the passing parameters like other code does.
>> > Or can we add check to return NULL when the passing name is NULL
>> > instead of crashing the kernel? Thanks.
>>
>> No.  Fix your driver.
>
> Let me elaborate on this.  Kernel code is supposed to be "reasonable".
> That means we don't check every argument to every function for sanity,
> unless it's going to cause trouble later.  Crashing immediately with
> a bogus argument is fine; you can see the problem and fix it immediately.
> Returning NULL with a bad argument is actually worse; you won't know why
> the function returned NULL (maybe we're out of memory?) and you'll have
> a more complex debugging experience.
>
> Sometimes it makes sense to accept a NULL pointer and do something
> reasonable, like kfree().  In this case, we can eliminate checks in all
> the callers.  But we don't, in general, put sanity checks on arguments
> without a good reason.
>
> Your reasons aren't good.  "The driver doesn't care" -- well, just pass
> the driver's name, then.

Thanks for your explanation. OK, force the driver to pass a pool name.
Sorry for noises.

-- 
Baolin.wang
Best Regards

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-05-31  2:34 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-30 11:28 [PATCH] mm: dmapool: Check the dma pool name Baolin Wang
2018-05-30 12:01 ` Matthew Wilcox
2018-05-30 12:14   ` Baolin Wang
2018-05-30 15:13     ` Matthew Wilcox
2018-05-30 15:41       ` Matthew Wilcox
2018-05-31  2:34         ` Baolin Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).