linux-kernel - Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

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: <CA+aCy1FDF7s-xRWkiDqxsVdNHgmoq_RrCTu14LrPtA6L0uLs2g@mail.gmail.com>
Date:	Tue, 3 May 2016 09:25:44 +0530
From:	Pranay Srivastava <pranjas@...il.com>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	mpa@...gutronix.de, nbd-general@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org
Subject: Re: [RESEND PATCH]nbd: fix might_sleep warning on socket shutdown

On Tue, May 3, 2016 at 5:45 AM, Greg KH <gregkh@...uxfoundation.org> wrote:
> On Mon, May 02, 2016 at 08:58:34AM +0530, Pranay Srivastava wrote:
>> Hi,
>>
>> Can the following patch be reviewed? I'm working on some more changes
>> on top of this change,
>> so it'll be really helpful if someone can review this patch and let me
>> know of shortcomings/issues
>> with this.
>>
>>
>> On Sat, Apr 30, 2016 at 11:49 AM, Pranay Kr. Srivastava
>> <pranjas@...il.com> wrote:
>> > This patch fixes the warning generated when a timeout occurs
>> > on the request and socket is closed from a non-sleep context
>> > by
>> >
>> > 1. Moving the socket closing on a timeout to nbd_thread_send
>> >
>> > 2. Make sock lock to be a mutex instead of a spin lock, since
>> >    nbd_xmit_timeout doesn't need to hold it anymore.
>> >
>> > 3. Move sock_shutdown outside the tx_lock in NBD_DO_IT.
>
> Why are you doing three things in one patch?
>
>> > ---
>> >  drivers/block/nbd.c | 85 +++++++++++++++++++++++++++++++----------------------
>> >  1 file changed, 50 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
>> > index 31e73a7..a52cc16 100644
>> > --- a/drivers/block/nbd.c
>> > +++ b/drivers/block/nbd.c
>> > @@ -3,7 +3,7 @@
>> >   *
>> >   * Note that you can not swap over this thing, yet. Seems to work but
>> >   * deadlocks sometimes - you can not swap over TCP in general.
>> > - *
>> > + *
>
> What did you change here?
>
>> >   * Copyright 1997-2000, 2008 Pavel Machek <pavel@....cz>
>> >   * Parts copyright 2001 Steven Whitehouse <steve@...gwyn.com>
>> >   *
>> > @@ -35,14 +35,14 @@
>> >  #include <linux/types.h>
>> >  #include <linux/debugfs.h>
>> >
>> > -#include <asm/uaccess.h>
>> > +#include <linux/uaccess.h>
>
> Why change this?
>
>> >  #include <asm/types.h>
>> >
>> >  #include <linux/nbd.h>
>> >
>> >  struct nbd_device {
>> >         u32 flags;
>> > -       struct socket * sock;   /* If == NULL, device is not ready, yet */
>> > +       struct socket *sock;    /* If == NULL, device is not ready, yet */
>
> You are mixing "code cleanup" changes with "change the logic" changes,
> please break this up into a series of patches, doing the code cleanup
> _last_ as you want to fix bugs first.
>

Ok. Will do that.

> thanks,
>
> greg k-h

Thanks alot Greg for review.

-- 
        ---P.K.S