Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750853AbWBFJcZ (ORCPT ); Mon, 6 Feb 2006 04:32:25 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1750850AbWBFJcZ (ORCPT ); Mon, 6 Feb 2006 04:32:25 -0500 Received: from silver.veritas.com ([143.127.12.111]:54035 "EHLO silver.veritas.com") by vger.kernel.org with ESMTP id S1750843AbWBFJcY (ORCPT ); Mon, 6 Feb 2006 04:32:24 -0500 Date: Mon, 6 Feb 2006 09:32:54 +0000 (GMT) From: Hugh Dickins X-X-Sender: hugh@goblin.wat.veritas.com To: Brian King cc: James Bottomley , Andrew Morton , linux-kernel@vger.kernel.org, linux-scsi@vger.kernel.org Subject: Re: [PATCH] ipr: don't doublefree pages from scatterlist In-Reply-To: <43E66FB6.6070303@us.ibm.com> Message-ID: References: <20060104172727.GA320@tau.solarneutrino.net> <20060105201249.GB1795@tau.solarneutrino.net> <20060109033149.GC283@tau.solarneutrino.net> <20060109185350.GG283@tau.solarneutrino.net> <20060118001252.GB821@tau.solarneutrino.net> <43E3D3EC.3040006@us.ibm.com> <43E66FB6.6070303@us.ibm.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-OriginalArrivalTime: 06 Feb 2006 09:32:23.0945 (UTC) FILETIME=[3C5E5F90:01C62B00] Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2138 Lines: 44 On Sun, 5 Feb 2006, Brian King wrote: > > No. I can't find any code in any architecture that modifies either the length, > page, or offset fields in the *_map_sg routines. >From looking at the source, the architectures I found to be doing scatterlist coalescing in some cases were alpha, ia64, parisc (some code under drivers), powerpc, sparc64 and x86_64. I agree with you that it would be possible for them to do the coalescing by just adjusting dma_address and dma_length (though it's architecture- dependent whether there are such fields at all), not interfering with the input page and length; and maybe some of them do proceed that way. I didn't find the coalescing code in any of them very easy to follow. So please examine arch/x86_64/kernel/pci_gart.c gart_map_sg (and dma_map_cont which it calls): x86_64 was the architecture on which the problem was really found with drivers/scsi/st.c, and avoided by that boot option iommu=nomerge. Lines like "*sout = *s;" and "*sout = sg[start];" are structure- copying whole scallerlist entries from one position in the list to another, without explicit mention of the page and length fields. > I'm still failing to see an actual bug in the ipr driver. Unless there is a > good reason for pushing additional complexity on every caller of pci_map_sg > to do additional bookkeeping, such as an architecture that is actually > modifying > fields in the scatterlist other than the dma_* fields, then I think it makes > more sense to clarify the API to say that pci_map_sg will not modify these > fields. The API is commendably clear that they may be modified. Like you I'd prefer it to be otherwise, and instead guarantee that they won't be: that would prevent this kind of surprise. But it's not how it is; and looking at the various pieces of coalescing code, I quickly abandoned my first inclination, to adjust them to make it so. Hugh - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/