linux-kernel - Re: [PATCH 1/2] dma-debug: add dma_debug_resize_entries() to adjust the number of dma_debug

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]

Date:	Wed, 15 Apr 2009 12:27:43 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	FUJITA Tomonori <fujita.tomonori@....ntt.co.jp>
Cc:	linux-kernel@...r.kernel.org, joerg.roedel@....com,
	akpm@...ux-foundation.org
Subject: Re: [PATCH 1/2] dma-debug: add dma_debug_resize_entries() to
	adjust the number of dma_debug_entries


* FUJITA Tomonori <fujita.tomonori@....ntt.co.jp> wrote:

> On Tue, 14 Apr 2009 12:58:23 +0200
> Ingo Molnar <mingo@...e.hu> wrote:
> 
> > 
> > * FUJITA Tomonori <fujita.tomonori@....ntt.co.jp> wrote:
> > 
> > > +int dma_debug_resize_entries(u32 num_entries)
> > > +{
> > > +	int i, delta, ret = 0;
> > > +	unsigned long flags;
> > > +	struct dma_debug_entry *entry;
> > > +
> > > +	spin_lock_irqsave(&free_entries_lock, flags);
> > > +
> > > +	if (nr_total_entries < num_entries) {
> > > +		delta = num_entries - nr_total_entries;
> > > +
> > > +		for (i = 0; i < delta; i++) {
> > > +			entry = kzalloc(sizeof(*entry), GFP_ATOMIC);
> > > +			if (!entry)
> > > +				break;
> > 
> > hm, using GFP_ATOMIC within a spinlock is not a very nice thing to 
> > do in general. While this is boot-only and the GFP_ATOMIC will 
> > likely succeed, this could become non-boot functionality and then 
> > it's exposed to the momentary VM pressure situation that might make 
> > GFP_ATOMIC fail.
> > 
> > Please fix this to be GFP_KERNEL.
> 
> Ok, fixed though I'm not sure it matters. It's unlikely that this 
> could become non-boot functionality so I chose a simple way.
> 
> I've attached a fixed patch. It's unlikely that this function is 
> called concurrently so I don't try hard.

thanks, it looks nice and clean now.

GFP_ATOMIC (mis-)use is prominently detected by Andrew's 
barf-o-meter so even though it's probably fine here it's still 
better to just not get into the habit. People would then see it in 
an otherwise tidy looking piece of code and think it's an example to 
follow.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/