Re: [PATCH] netconsole: release the spinlock before __netpoll_cleanup()

[Veaceslav Falico <darkmag@gmail.com>]

On Sun, Mar 10, 2013 at 4:25 PM, Veaceslav Falico <vfalico@redhat.com> wrote:
> On Thu, Mar 07, 2013 at 04:14:38PM -0500, David Miller wrote:
>>
...snip...
>> Quite a bit of email corruption of this patch.
>
>
> Sorry, somehow messed it.
>
>
>>
>> Also, this code block is probably too deeply indented to be sane,
>> consider creating a small helper function to call instead.
>
>
> It gets quite ugly if I try to move it to another function. However, maybe
> something like that will work - it's effectively the same code, just that
> I've moved the long part out of the if () { } block. Looks a lot more
> readable, though one line still breaks 80chars limit. I've reworked the
> subject/commit message too.
>
> Subject: [PATCH] netconsole: don't call __netpoll_cleanup() while atomic
>
> __netpoll_cleanup() is called in netconsole_netdev_event() while holding a
> spinlock. Release/acquire the spinlock before/after it and restart the
>
> loop.
>
> Signed-off-by: Veaceslav Falico <vfalico@redhat.com>
> ---
>    drivers/net/netconsole.c |   22 +++++++++++++++-------
>    1 files changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 37add21..38eaa8c 100644
>
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -666,6 +666,7 @@ static int netconsole_netdev_event(struct notifier_block
> *this,
>                 goto done;
>         spin_lock_irqsave(&target_list_lock, flags);
> +restart:
>         list_for_each_entry(nt, &target_list, list) {
>                 netconsole_target_get(nt);
>                 if (nt->np.dev == dev) {
> @@ -679,14 +680,21 @@ static int netconsole_netdev_event(struct
> notifier_block *this,
>                                 /*
>
>                                  * rtnl_lock already held
>                                  */
> -                               if (nt->np.dev) {
> -                                       __netpoll_cleanup(&nt->np);
> -                                       dev_put(nt->np.dev);
> -                                       nt->np.dev = NULL;
> +                               if (!nt->np.dev) {
> +                                       nt->enabled = 0;
> +                                       stopped = true;
> +                                       break;
>                                 }
> -                               nt->enabled = 0;
> -                               stopped = true;
> -                               break;
> +                               /*
> +                                * we might sleep in __netpoll_cleanup()
> +                                */
> +                               spin_unlock_irqrestore(&target_list_lock,
> flags);
> +                               __netpoll_cleanup(&nt->np);
> +                               spin_lock_irqsave(&target_list_lock, flags);
> +                               dev_put(nt->np.dev);
> +                               nt->np.dev = NULL;
>
> +                               netconsole_target_put(nt);
> +                               goto restart;
>                         }
>                 }
>                 netconsole_target_put(nt);
> --
> 1.7.1

Self-NAK this patch, I've triggered another kernel panic with it. Will
send another one shortly. Basicly, the whole if (!nt->np.dev) is not
needed and nt->enabled=0 should always be set, otherwise we
end up with nt->np.dev == NULL and nt->enabled == 1, thus
triggering panics in places like write_msg(), where it verifies only
if the nt->enabled is true.

-- 
Best regards,
Veaceslav Falico