linux-kernel - Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org for pnp devices with no ctl

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.1.10.0805291403030.3873@woody.linux-foundation.org>
Date:	Thu, 29 May 2008 14:13:26 -0700 (PDT)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
cc:	Jeff Garzik <jeff@...zik.org>, linux-kernel@...r.kernel.org,
	linux-ide@...r.kernel.org
Subject: Re: RESEND: [PATCH] libata-sff: Fix oops reported in kerneloops.org
 for pnp devices with no ctl

On Thu, 29 May 2008, Alan Cox wrote:
>
> And this is how a revised one would look.

Ok, this looks nicer, but how about:

> +static u8 ata_sff_irq_status(struct ata_port *ap)
> +{
> +	u8 status;
> +
> +	if (ap->ops->sff_check_altstatus || ap->ioaddr.altstatus_addr) {
> +		status = ata_sff_altstatus(ap);

This here is all about not oopsing in ata_sff_altstatus(), ie just knowing 
about the bug.

Why not just fix ata_sff_altstatus(), instead of causing these kinds of 
problems downstream?

If you don't want to read the status register, fine. But at least do 
something like

 - make ata_sff_altstatus static, since it's useless and dangerous to call 
   otherwise (ie make the exported interfaces be the nicer higher-level 
   ones that are sane)

 - and make it just return 0 if the altstatus register doesn't exist.

At that point, both 'ata_sff_irq_status()' and 'ata_sff_sync()' can just 
use ata_sff_altstatus() directly, *without* having to check that altstatus 
setup by hand, or re-implement the function just because it was buggy and 
broken to begin with.

So now ata_sff_irq_status() just becomes

	static u8 ata_sff_irq_status(struct ata_port *ap)
	{
		u8 status;

		status = ata_sff_altstatus(ap);
		if (status & ATA_BUSY)
			return status;

		/* Clear INTRQ latch */
		status = ata_sff_check_status(ap);
		return status;
	}

and if there was no altstatus register, everything is fine because 
"ata_sff_altstatus()" was safe and returned 0 (and not ATA_BUSY).

Or feel free and make it return ATA_INVALID, which has a value of 0x100, 
or something - it still won't be busy, and it will clearly not be a byte 
read, so people *can* check for "no altstatus existed" if they want to.

Similarly, 'ata_sff_sync()' just becomes

	void ata_sff_sync(struct ata_port *ap)
	{
		ata_sff_altstatus();
	}

and 'ata_sff_pause()' becomes

	void ata_sff_pause(struct ata_port *ap)
	{
		ata_sff_altstatus();
		ndelay(400);
	}

and again, if there is no altstatus register, that's a low-level driver 
issue.

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