linux-kernel - Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata

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

Date:	Tue, 20 May 2008 20:17:08 -0600
From:	Robert Hancock <hancockr@...w.ca>
To:	Arjan van de Ven <arjan@...radead.org>
Cc:	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv

Arjan van de Ven wrote:
> From: Arjan van de Ven <arjan@...ux.intel.com>
> Subject: [PATCH] ata: fix sleep-while-holding-spinlock in sata_nv
> 
> blk_queue_bounce_limit() is a sleeping function, so reorganize
> the code a little to not call it while holding a spinlock.
> 
> Signed-off-by: Arjan van de Ven <arjan@...ux.intel.com>
> ---
>  drivers/ata/sata_nv.c |    8 +++++---
>  1 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
> index 858f706..a7bc56d 100644
> --- a/drivers/ata/sata_nv.c
> +++ b/drivers/ata/sata_nv.c
> @@ -671,6 +671,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
>  	port1 = ap->host->ports[1]->private_data;
>  	sdev0 = ap->host->ports[0]->link.device[0].sdev;
>  	sdev1 = ap->host->ports[1]->link.device[0].sdev;
> +
> +	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
> +	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
> +	spin_unlock_irqrestore(ap->lock, flags);
> +
>  	if ((port0->flags & NV_ADMA_ATAPI_SETUP_COMPLETE) ||
>  	    (port1->flags & NV_ADMA_ATAPI_SETUP_COMPLETE)) {
>  		/** We have to set the DMA mask to 32-bit if either port is in
> @@ -701,14 +706,11 @@ static int nv_adma_slave_config(struct scsi_device *sdev)
>  					       pp->adma_dma_mask);
>  	}
>  
> -	blk_queue_segment_boundary(sdev->request_queue, segment_boundary);
> -	blk_queue_max_hw_segments(sdev->request_queue, sg_tablesize);
>  	ata_port_printk(ap, KERN_INFO,
>  		"DMA mask 0x%llX, segment boundary 0x%lX, hw segs %hu\n",
>  		(unsigned long long)*ap->host->dev->dma_mask,
>  		segment_boundary, sg_tablesize);
>  
> -	spin_unlock_irqrestore(ap->lock, flags);
>  
>  	return rc;
>  }

I'm not certain this is safe to do it quite this way. It would be better 
to keep that spinlock held so that no operations could be in progress on 
either port while these operations are happening.

It would be better to fix the regression from 
419c434c35614609fd0c79d335c134bf4b88b30b in block/blk_settings.c that 
resulted in the blk_queue_bounce_limit allocation wrongly allocating 
emergency ISA pages in the first place as a 32-bit DMA mask does not 
need them.
--
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/