From mboxrd@z Thu Jan 1 00:00:00 1970 From: HAYASAKA Mitsuo Subject: Re: [RFC PATCH net 1/2] [BUGFIX] bonding: use local function pointer of bond->recv_probe in bond_handle_frame Date: Wed, 12 Oct 2011 16:04:44 +0900 Message-ID: <4E953C0C.9020107@hitachi.com> References: <20111007124925.7089.27260.stgit@ltc219.sdl.hitachi.co.jp> <20111007124954.7089.9776.stgit@ltc219.sdl.hitachi.co.jp> <4E943E53.6010601@hitachi.com> <1318339433.2538.25.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: =?UTF-8?B?QW3DqXJpY28gV2FuZw==?= , Jay Vosburgh , Andy Gospodarek , netdev@vger.kernel.org, linux-kernel@vger.kernel.org, yrl.pp-manager.tt@hitachi.com To: Eric Dumazet Return-path: In-Reply-To: <1318339433.2538.25.camel@edumazet-HP-Compaq-6005-Pro-SFF-PC> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org Hi Eric, Thank you for your comment. (2011/10/11 22:23), Eric Dumazet wrote: > Le mardi 11 octobre 2011 =C3=A0 22:02 +0900, HAYASAKA Mitsuo a =C3=A9= crit : >> Hi WANG Cong >> >> Thank you for your comments. >> >> (2011/10/07 22:24), Am=C3=A9rico Wang wrote: >>> On Fri, Oct 7, 2011 at 8:49 PM, Mitsuo Hayasaka >>> wrote: >>>> The bond->recv_probe is called in bond_handle_frame() when >>>> a packet is received, but bond_close() sets it to NULL. So, >>>> a panic occurs when both functions work in parallel. >>>> >>>> Why this happen: >>>> After null pointer check of bond->recv_probe, an sk_buff is >>>> duplicated and bond->recv_probe is called in bond_handle_frame. >>>> So, a panic occurs when bond_close() is called between the >>>> check and call of bond->recv_probe. >>>> >>>> Patch: >>>> This patch uses a local function pointer of bond->recv_probe >>>> in bond_handle_frame(). So, it can avoid the null pointer >>>> dereference. >>>> >>> >>> Hmm, I don't doubt it can fix the problem, I am wondering if >>> bond->recv_probe should be protected by bond->lock... >> >> Indeed, in general any resources should be protected from the asynch= ronous >> workers. >> >> At first, I thought it should be handled with lock protection, as we= ll. >> However, I guess that using bond->lock on this kind of hot-path may >> introduces unnecessary overhead. In addition, this code works well >> without the strict lock protection. So, I think this change is the >> right way to fix it. >=20 > Maybe, but then ACCESS_ONCE() is needed to prevent compiler to > 'optimize' the temporary variable. >=20 > Or use rcu_dereference() to make the whole thing really safe and self > documented. >=20 I agreed. I'd like to send the patch with ACCESS_ONCE(), again. Thanks.