linux-kernel - Re: [PATCH v2 3/8] dma-debug: Refactor 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:   Thu, 6 Dec 2018 18:10:47 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Christoph Hellwig <hch@....de>
Cc:     m.szyprowski@...sung.com, iommu@...ts.linux-foundation.org,
        linux-kernel@...r.kernel.org, cai@....us, salil.mehta@...wei.com,
        john.garry@...wei.com
Subject: Re: [PATCH v2 3/8] dma-debug: Refactor dma_debug_entry allocation

On 06/12/2018 14:23, Christoph Hellwig wrote:
>> +static int dma_debug_add_entries(u32 num_entries, gfp_t gfp)
>> +{
>> +	struct dma_debug_entry *entry, *next_entry;
>> +	LIST_HEAD(tmp);
>> +	int i;
>> +
>> +	for (i = 0; i < num_entries; ++i) {
>> +		entry = kzalloc(sizeof(*entry), gfp);
>> +		if (!entry)
>> +			goto out_err;
>> +
>> +		list_add_tail(&entry->list, &tmp);
>> +	}
>> +
>> +	list_splice(&tmp, &free_entries);
>> +	num_free_entries += num_entries;
>> +	nr_total_entries += num_entries;
> 
> The adding to a local list and splicing seems a bit pointless if we
> do it all under lock anyway.  Either we change the locking in
> dma_debug_resize_entries and your upcoming automatic allocation that
> we only do it over the splice and counter adjustment, which would
> have the advantage of allowing freeing of entries in parallel to these
> allocations, or we could just drop the local tmp list.

AFAICS the tmp list wasn't about locking as much as meaning that if 
kzalloc() failed at any point, we can free the partial allocation and 
back out without disturbing free_entries at all - that still makes sense 
to me up until patch #8 where we embrace the "never free anything" 
paradigm and rip out the final traces.

That said, maybe I should just drop the refactoring of 
dma_debug_resize_entries() now that I'm deleting it as part of the same 
series anyway - then I guess I squash what's left of this patch into #4 
and bring forward some of the simplification from #8 to start with. 
Would that be more agreeable?

Robin.