linux-kernel - Re: [PATCH 14/27] drivers/scsi: Use memdup

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

Message-Id: <20100525152323.26652def.akpm@linux-foundation.org>
Date:	Tue, 25 May 2010 15:23:23 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	James Bottomley <James.Bottomley@...e.de>
Cc:	Boaz Harrosh <bharrosh@...asas.com>, Julia Lawall <julia@...u.dk>,
	Doug Gilbert <dgilbert@...erlog.com>,
	linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 14/27] drivers/scsi: Use memdup_user

On Tue, 25 May 2010 09:58:33 -0500
James Bottomley <James.Bottomley@...e.de> wrote:

> On Tue, 2010-05-25 at 17:15 +0300, Boaz Harrosh wrote:
> > On 05/23/2010 07:22 PM, James Bottomley wrote:
> > > On Sun, 2010-05-23 at 18:36 +0300, Boaz Harrosh wrote:
> > >>> This type of transformation has really no value at all.

kerrrrapp!!  There's always value in replacing open-coded straggles
with calls to well-understood, well-documented, well-tested library
code.

>  The code you're
> > >>> proposing to replace is already correct.  I'm fairly ambivalent on
> > >>> patterned APIs anyway but I accept they're useful way to prevent new
> > >>> code getting it wrong. However, it's completely bogus to force
> > >>> replacement of correctly functioning code throughout the kernel

Hello?  This is Linux.  We're continuously filtering through old code,
improving and updating it.  There's nothing which exempts
./drivers/scsi/ from our reasons for doing this.

> (unless
> > >>> you're going to argue that everyone who tries to copy from user into a
> > >>> kmalloc space does a cut and paste from sg?)
> > >>>
> > >>> Of infinitely greater service would be finding any places where the
> > >>> pattern is being incorrectly used.
> > >>>
> > >>
> > >> It looks like it is not done 100% kosher and calling memdup_user should
> > >> be better.
> > >>
> > >> - For 1 memdup_user does a GFP_KERNEL with a comment on how copy_from_user
> > >>   would eventually sleep, so what's the point of GFP_ATOMIC?
> > > 
> > > Well, since you've written a storage driver, I really hope that question
> > > is rhetorical ...
> > > 
> > 
> > Not really I do mean it in this case. It is not a storage-driver code
> > path. Is sg a storage-driver. I was not aware anyone used it for one.
> 
> by storage driver, I mean anything in the writeout path ... this
> includes ULDs, of which sg is one.
> 
> > > The reason for using GFP_ATOMIC from user context in storage drivers is
> > > to avoid writeout deadlock:  you don't want to trigger a swap write
> > > while you potentially occupy the writeout path.  In all older drivers
> > > this had to be GFP_ATOMIC because GFP_NOIO wasn't around.
> > > 
> > 
> > I confess my total swap ignorance. I was under the impression that
> > swap needs a block device. OK you could do a user-mode filesystem
> > over sg, under loop device. Does swap work over loop device=>ext2
> > for example? Or is there a way to directly swap over a filesystem
> > (Not block based)?
> 
> Well, swap can function above block devices or files.
> 
> However, it's not just swap, it's also triggerable via mmap (you do file
> backed mmap of a large area then have a process that joyfully dirties
> lots of it) ... and that's what can make it such a difficult problem.
> Basically you end up with a system with mostly dirty memory that it has
> to clean via writeout (either to swap or by flushing dirty pages).  The
> writeout path that these pages go down cannot ever wait on dirty pages
> being flushed (which is what a sleeping kmalloc does), because that can
> end up as a deadlock.

copy_from_user() can take a fault, take a zillion locks, perform
GFP_HIGHUSER allocations, enter page reclaim, enter filesystems, etc. 
View a pagefault as a syscall with a different calling convention -
it's that high level.

If this driver if correct in potentially taking a pagefault at this
place then the patch is correct and desirable.  Otherwise, well, the
driver already has problem and the patch did nothing to increase them.

And, of course, Julia's "no value" work caused us to identify those
problems.

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