Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <c8013f0894299c6aedfbcd5e35f5fa33b858d915.camel@mendozajonas.com>
Subject: Re: [PATCH net-next] net/ncsi: Avoid GFP_KERNEL in response handler
From:   Samuel Mendoza-Jonas <sam@mendozajonas.com>
To:     Eric Dumazet <eric.dumazet@gmail.com>, netdev@vger.kernel.org
Cc:     "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org
Date:   Fri, 01 Jun 2018 10:33:27 +1000
In-Reply-To: <69fcb143-00a2-2ddf-e2d4-c692b650f292@gmail.com>
References: <20180531070254.28878-1-sam@mendozajonas.com>
         <69fcb143-00a2-2ddf-e2d4-c692b650f292@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 2018-05-31 at 04:50 -0400, Eric Dumazet wrote:
> 
> On 05/31/2018 03:02 AM, Samuel Mendoza-Jonas wrote:
> > ncsi_rsp_handler_gc() allocates the filter arrays using GFP_KERNEL in
> > softirq context, causing the below backtrace. This allocation is only a
> > few dozen bytes during probing so allocate with GFP_ATOMIC instead.
> > 
> 
> Hi Samuel
> 
> You forgot to add
> 
> Fixes: 062b3e1b6d4f ("net/ncsi: Refactor MAC, VLAN filters")
> 
> size = (rsp->uc_cnt + rsp->mc_cnt + rsp->mixed_cnt) * ETH_ALEN;
> 
> -> seems to be able to reach more than few dozen bytes...

Hi Eric,

The NCSI spec (at least in the v1.1.0 version I'm looking at) sets the
total number of MAC address filters at 8, so we would be looking at a
maximum of 8 * ETH_ALEN = 48 bytes.
That said it shouldn't be too arduous to move the allocation to later in
the probe/configure cycle so if needed we could do that.

> 
> Also, what prevents ncsi_rsp_handler_gc() to be called multiples times ?
> 
> nc->mac_filter.addrs & nc->vlan_filter.vids would be re-allocated and memory would leak.
> 

Good point, we should put a check there just in case to see if it's
allocated. We should be safe though as ncsi_rsp_handler_gc() should only
be called via ncsi_probe_channel() which only happens through
ncsi_start_dev(), and addrs/vids is cleaned up in ncsi_remove_channel().
Rogue packets shouldn't hit the ncsi_rsp_handler_gc() handler without an
outstanding request.. but it probably is safer to check regardless.

Regards,
Sam