linux-kernel - Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and ->ata_*put

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:	Tue, 8 Apr 2008 11:40:29 +0200
From:	Richard Zidlicky <rz@...ux-m68k.org>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	Geert Uytterhoeven <geert@...ux-m68k.org>,
	linux-ide@...r.kernel.org,
	Linux Kernel Development <linux-kernel@...r.kernel.org>,
	Linux/m68k <linux-m68k@...r.kernel.org>,
	Michael Schmitz <schmitz@...ian.org>
Subject: Re: [PATCH 5/5] falconide/q40ide: add ->atapi_*put_bytes and
	->ata_*put_data methods

On Mon, Apr 07, 2008 at 09:13:42PM +0200, Bartlomiej Zolnierkiewicz wrote:

> 
> v2:
> * Add 'struct request *rq' argument to ->ata_{in,out}put_data methods
>   and don't byte-swap disk fs requests (we shouldn't un-swap fs requests
>   because fs itself is stored byte-swapped on the disk) - this is how
>   things were done before the patch (ideally device mapper should be
>   used instead but it would break existing setups and would have some
>   performance impact).

I like the part about checking 'struct request *rq', will make a  clean
distinction between ide command data and ide disk/medium data which is badly
needed.

However, not only FS data is byteswapped, complete disk including partition
table and everything else is. Will "rq->cmd_type == REQ_TYPE_FS" also catch
all these cases?



> Index: b/drivers/ide/legacy/falconide.c
> ===================================================================
> --- a/drivers/ide/legacy/falconide.c
> +++ b/drivers/ide/legacy/falconide.c
> @@ -44,6 +44,36 @@
>  int falconide_intr_lock;
>  EXPORT_SYMBOL(falconide_intr_lock);
>  
> +static void falconide_atapi_input_bytes(ide_drive_t *drive, void *buf,
> +					unsigned int len)
> +{
> +	insw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
> +}
> +
> +static void falconide_atapi_output_bytes(ide_drive_t *drive, void *buf,
> +					 unsigned int len)
> +{
> +	outsw_swapw(drive->hwif->io_ports.data_addr, buf, (len + 1) / 2);
> +}
> +
> +static void falconide_ata_input_data(ide_drive_t *drive, struct request *rq,
> +				     void *buf, unsigned int wcount)
> +{
> +	if (drive->media == ide_disk && rq && rq->cmd_type == REQ_TYPE_FS)
> +		return insw(drive->hwif->io_ports.data_addr, buf, wcount * 2);
> +
> +	falconide_atapi_input_bytes(drive, buf, wcount * 4);
> +}

this will still do double swapping for disk access, although this is very
easier to fix once the distinction between ide and disk data works fine.

So IMHO despite the little concerns this is the way to go.

Richard
--
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/