linux-kernel - Re: libata: use WARN_ON

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>] [day] [month] [year] [list]

Message-ID: <20090109213216.19889bdb@lxorguk.ukuu.org.uk>
Date:	Fri, 9 Jan 2009 21:32:16 +0000
From:	Alan Cox <alan@...rguk.ukuu.org.uk>
To:	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Cc:	torvalds@...ux-foundation.org, tj@...nel.org
Subject: Re: libata: use WARN_ON_ONCE on hot paths

On Fri, 9 Jan 2009 21:00:28 GMT
Linux Kernel Mailing List <linux-kernel@...r.kernel.org> wrote:

Most of these look ok but several should never have been WARN in the
first place:

@@ -4556,7 +4556,7 @@ void ata_sg_clean(struct ata_queued_cmd
*qc)
>  	struct scatterlist *sg = qc->sg;
>  	int dir = qc->dma_dir;
>  
> -	WARN_ON(sg == NULL);
> +	WARN_ON_ONCE(sg == NULL);

If that ever occurs we are in deep deep doodo - this should be a BUG() so
we stop the box before we do serious damage.

> @@ -651,7 +651,7 @@ void ata_sff_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
>  			iowrite8(tf->ctl, ioaddr->ctl_addr);
>  			ap->last_ctl = tf->ctl;
>  		} else
> -			WARN_ON(1);
> +			WARN_ON_ONCE(1);

This one should always have been a BUG(). At the point we hit this (which
we never have yet) the returned data for the command executed is *lost*
and we return garbage. It's a case that will only trigger due to a clear
internal bug (where we want the driver author to catch it promptly) or
memory corruption. It's not something that will pop up from a quirky bit
of timing.

> @@ -891,7 +891,7 @@ static void ata_pio_sectors(struct ata_queued_cmd *qc)
>  		/* READ/WRITE MULTIPLE */
>  		unsigned int nsect;
>  
> -		WARN_ON(qc->dev->multi_count == 0);
> +		WARN_ON_ONCE(qc->dev->multi_count == 0);

Ditto should be a BUG().

> -	WARN_ON(qc->dev->cdb_len < 12);
> +	WARN_ON_ONCE(qc->dev->cdb_len < 12);

If this ever occurs we are writing garbage to the ATAPI device and
continuing is a very bad idea. It could remain a warn but the command
issue should be aborted

(Should be a WARN/return)
	if (qc->dev->cdb_len < 12) {
		WARN_ON_ONCE(...)
		return;

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