netdev - Re: [PATCH] net: mvpp2: sleeping function called from invalid context

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

Message-ID: <20170408151855.0a7c89b9@free-electrons.com>
Date:   Sat, 8 Apr 2017 15:18:55 +0200
From:   Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
To:     Gerald Guillaume <gerald.guillaume@...il.com>
Cc:     netdev@...r.kernel.org, Mirko Lindner <mlindner@...vell.com>,
        Stephen Hemminger <stephen@...workplumber.org>,
        David Miller <davem@...emloft.net>,
        Marcin Wojtas <mw@...ihalf.com>,
        Ken Wilson <ken.wilson@...ngear.com>
Subject: Re: [PATCH] net: mvpp2: sleeping function called from invalid
 context

Hello,

Thanks for your patch! However, it is badly line wrapped, you should
consider sending your patch with "git send-email".

On Sat, 8 Apr 2017 15:07:14 +0200, Gerald Guillaume wrote:

> --- linux-3.18/drivers/net/ethernet/marvell/mvpp2.c     2017-04-03
> 10:29:31.863264347 +0200
> +++ linux-3.x/drivers/net/ethernet/marvell/mvpp2.c      2017-04-03
> 10:45:23.008339453 +0200
> @@ -2997,7 +2997,7 @@
>         struct mvpp2_prs_entry *pe;
>         int tid;
> 
> -       pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +       pe = kzalloc(sizeof(*pe), GFP_ATOMIC);
>         if (!pe)
>                 return NULL;
>         mvpp2_prs_tcam_lu_set(pe, MVPP2_PRS_LU_MAC);
> @@ -3059,7 +3059,7 @@
>                 if (tid < 0)
>                         return tid;
> 
> -               pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> +               pe = kzalloc(sizeof(*pe), GFP_ATOMIC);
>                 if (!pe)
>                         return -1;
>                 mvpp2_prs_tcam_lu_set(pe, MVPP2_PRS_LU_MAC);

I am wondering if doing a GFP_ATOMIC allocation for this is the right
solution. Should it be pre-allocated instead?

I would have to look at how other drivers typically do this.

Perhaps allocating on the stack is reasonable? After all, sizeof(struct
mvpp2_prs_entry) is only: 4 + 6 * 4 + 4 * 4 = 44 bytes. It's allocated
at the beginning of the function, and freed at the end, so it really
calls for a local variable.

Anyway, I think it's worth investigating a different solution than
blindly converting to a GFP_ATOMIC allocation.

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com