Looks ok to me too. But I suggest we let David apply the pending patch as is to net, as that code is still wrong, and then post this improvement to net-next later.
///jon > -----Original Message----- > From: Tommi Rantala [mailto:tommi.t.rant...@nokia.com] > Sent: Friday, December 01, 2017 09:03 > To: Ying Xue <ying....@windriver.com>; Jon Maloy > <jon.ma...@ericsson.com>; David S. Miller <da...@davemloft.net>; > netdev@vger.kernel.org; tipc-discuss...@lists.sourceforge.net; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH net v2] tipc: call tipc_rcv() only if bearer is up in > tipc_udp_recv() > > 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); > >
