From mboxrd@z Thu Jan 1 00:00:00 1970 From: Samuel Mendoza-Jonas Subject: Re: [PATCH net-next] net/ncsi: Avoid GFP_KERNEL in response handler Date: Fri, 01 Jun 2018 10:33:27 +1000 Message-ID: References: <20180531070254.28878-1-sam@mendozajonas.com> <69fcb143-00a2-2ddf-e2d4-c692b650f292@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: "David S . Miller" , linux-kernel@vger.kernel.org, openbmc@lists.ozlabs.org To: Eric Dumazet , netdev@vger.kernel.org Return-path: In-Reply-To: <69fcb143-00a2-2ddf-e2d4-c692b650f292@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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