linux-kernel - Re: [PATCH] scsi: debug: fix type mismatch warning for sg_pcopy_from

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]

Message-ID: <555DBD06.7060601@intel.com>
Date:	Thu, 21 May 2015 12:09:58 +0100
From:	Dave Gordon <david.s.gordon@...el.com>
To:	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>
CC:	"Martin K. Petersen" <martin.petersen@...cle.com>,
	Akinobu Mita <akinobu.mita@...il.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	jbottomley@...n.com
Subject: Re: [PATCH] scsi: debug: fix type mismatch warning for sg_pcopy_from_buffer

On 20/05/15 21:31, Arnd Bergmann wrote:
> On Wednesday 20 May 2015 12:53:29 Andrew Morton wrote:
>> On Tue, 19 May 2015 23:22:39 +0200 Arnd Bergmann <arnd@...db.de> wrote:
>>>
>>> I can't decide if this is actually a good idea, or if we should rather drop
>>> the sg_pcopy_from_buffer() patch. Maybe someone else sees a better solution.
>>
>> Could make do_device_access() call sg_copy_buffer() directly.
>>
>> But yes, dropping the sg_pcopy_from/to_buffer changes is reasonable. 
>> sg_copy_buffer() is bidirectional and that won't be changing, so
>> putting constified wrapeprs around it is kinda fake.
> 
> Ok. The part I only saw now is that do_device_access() is the only user
> of sg_pcopy_from_buffer(), so if that passes a non-const argument,
> there is dropping the patch will be teh best solution.
> 
> 	Arnd

do_device_access() may the only user of sg_pcopy_from_buffer() in the
-mm tree at the moment, but the const-patch was created because we were
using the sg_pcopy_{to,from}_buffer functions in new code in the i915
driver (published to the intel-gfx mailing list, but not yet folded into
the upstream versions). So quite soon it won't be the only user :)

The various sg_[p]copy_* wrappers all just supply trailing parameters
for the convenience of those who don't need (and don't want to deal
with) the full capabilities of the underlying sg_copy_buffer(). In
particular, we want the wrappers for the benefit of users that *don't*
use this flag-specifies-direction style (which I think is actually quite
rare and not really conducive to robust checking). The
separate-source-and-destination style seems much more common (cf. memcpy()).
And scsi_debug.c itself has functions fill_from_dev_buffer() and
fetch_to_dev_buffer() that call the separate sg_copy_{to,from}_buffer()
wrappers.

But since do_device_access() has the same parameter style as
sg_copy_buffer() (i.e. pointer parameters that may be either source or
destination, plus a flag to specify direction of transfer, as opposed to
one (const *) parameter for the input and a separate one for the
(non-const) destination), I think it quite reasonable that
do_device_access() should call sg_copy_buffer() directly rather than
going through one or other wrapper. In fact it simplifies the code
further; we can lose four lines and get rid of the function pointer
entirely, just by passing 'do_write' down to sg_copy_buffer(). See
attached patch :)

.Dave.

View attachment "0001-scsi-resolve-sg-buffer-const-ness-issue.patch" of type "text/x-patch" (2473 bytes)