linux-kernel - Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O

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, 2 May 2007 20:29:04 +0100
From:	Christoph Hellwig <hch@...radead.org>
To:	Stefan Richter <stefanr@...6.in-berlin.de>
Cc:	linux-kernel@...r.kernel.org, Kristian H??gsberg <krh@...hat.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	linux1394-devel <linux1394-devel@...ts.sourceforge.net>,
	Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH 2/6] firewire: isochronous and asynchronous I/O

> +	for (i = 0; i < buffer->page_count; i++) {
> +		buffer->pages[i] = alloc_page(GFP_KERNEL | GFP_DMA32 | __GFP_ZERO);
> +		if (buffer->pages[i] == NULL)
> +			goto out_pages;
> +
> +		address = dma_map_page(card->device, buffer->pages[i],
> +				       0, PAGE_SIZE, direction);
> +		if (dma_mapping_error(address)) {
> +			__free_page(buffer->pages[i]);
> +			goto out_pages;
> +		}

Are you sure using streaming dma mapping is safe here?  I don't see
actual user in this patch, but doing the proper ownership protocol
for them is quite difficult if you reuse them, and allocating them
in kernelspace usually means you want to keep reusing them.

> +#include <linux/kthread.h>

You don't actually seem to use this one ..

> +#include <asm/uaccess.h>

.. or this one ..

> +#include <asm/semaphore.h>

.. or this one.

> +	retval = fw_core_add_address_handler(&topology_map,
> +					     &topology_map_region);
> +	BUG_ON(retval < 0);
> +
> +	retval = fw_core_add_address_handler(&registers,
> +					     &registers_region);
> +	BUG_ON(retval < 0);
> +
> +	/* Add the vendor textual descriptor. */
> +	retval = fw_core_add_descriptor(&vendor_id_descriptor);
> +	BUG_ON(retval < 0);
> +	retval = fw_core_add_descriptor(&model_id_descriptor);
> +	BUG_ON(retval < 0);

These kinds of bug checks look wrong.  Either the operations
can't fail in which case they should not return an error value
or you should handle them properly.

Both the previous and this patch contain quite a lot of GFP_ATOMIC
allocation which are a sign of not having a very good layering.
-
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/