From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tommi Rantala Subject: Re: [PATCH net v2] tipc: call tipc_rcv() only if bearer is up in tipc_udp_recv() Date: Fri, 1 Dec 2017 16:02:47 +0200 Message-ID: References: <20171129104842.30781-1-tommi.t.rantala@nokia.com> <213f64e0-d5c3-511f-b83a-c468c399abc1@windriver.com> <840ce8d1-7271-5189-4cfa-e40f4a0a4172@windriver.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit To: Ying Xue , Jon Maloy , "David S. Miller" , netdev@vger.kernel.org, tipc-discussion@lists.sourceforge.net, linux-kernel@vger.kernel.org Return-path: In-Reply-To: <840ce8d1-7271-5189-4cfa-e40f4a0a4172@windriver.com> Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On 01.12.2017 15:18, Ying Xue wrote: > On 11/30/2017 08:32 PM, Tommi Rantala wrote: >>> In my opinion, the real root cause of the issue is because we too early >>> set a not-yet-initialized bearer instance to ub->bearer through >>> rcu_assign_pointer(ub->bearer, b) in tipc_udp_enable(). Instead if we >>> assign the bearer pointer at the end of tipc_udp_enable() where the >>> bearer has been completed the initialization, the issue would be avoided. >> Hi, sorry, I fail to see how that helps. >> >> bearer->tolerance is only initialized in tipc_enable_bearer() after >> calling m->enable_media() ie. tipc_udp_enable(). >> >> So even if we do "rcu_assign_pointer(ub->bearer, b)" later in >> tipc_udp_enable(), bearer->tolerance will still be uninitialized, and >> the crash can happen. > > Sorry, I missed the point that b->tolerance is not uninitialized when we > assign bearer pointer to ub->bearer later. > > But in my view the issue happened is because we enable media too early. > So it's better to change the code as belows: Thanks, looks good to me! Tested in 4.4 (which does not have b->up), and this fixes the oops also there. -Tommi > diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c > index 47ec121..ec6f02a 100644 > --- a/net/tipc/bearer.c > +++ b/net/tipc/bearer.c > @@ -320,19 +320,18 @@ static int tipc_enable_bearer(struct net *net, > const char *name, > > strcpy(b->name, name); > b->media = m; > - res = m->enable_media(net, b, attr); > - if (res) { > - pr_warn("Bearer <%s> rejected, enable failure (%d)\n", > - name, -res); > - return -EINVAL; > - } > - > b->identity = bearer_id; > b->tolerance = m->tolerance; > b->window = m->window; > b->domain = disc_domain; > b->net_plane = bearer_id + 'A'; > b->priority = priority; > + res = m->enable_media(net, b, attr); > + if (res) { > + pr_warn("Bearer <%s> rejected, enable failure (%d)\n", > + name, -res); > + return -EINVAL; > + } > test_and_set_bit_lock(0, &b->up); >