Re: [PATCH 14/27] drivers/scsi: Use memdup_user

[James Bottomley]

On Sun, 2010-05-23 at 18:36 +0300, Boaz Harrosh wrote:
> > This type of transformation has really no value at all.  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 (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 ...

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.

This also illustrates the problem with design patterns:  The idea that
if you have user context, you must be able to kmalloc GFP_KERNEL seems
logical to the people who wrote the pattern, but actually it's
potentially incorrect for storage.

Now in the particular case of sg, I don't believe we'll ever want to
swap over sg (famous last words, of course), so in this instance we
probably could get away with using GFP_KERNEL ... but since it's
following the storage pattern, GFP_ATOMIC (or GFP_NOIO) is correct.

Does osd need auditing for this problem (or would no-one ever do swap
over osd)?

> If this is by design then it surly calls for a comment that explains.
> (I would like to know)

This pattern occurs many times in storage ... documenting it at every
callsite would be a huge waste.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/