Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1227866

Re: [PATCH v3] netlink: Fix autobind race condition that leads to zero port ID

Show all headers | View raw

Hello, Herbert.

On Fri, Sep 18, 2015 at 02:36:10PM +0800, Herbert Xu wrote:
> On Thu, Sep 17, 2015 at 07:30:34AM -0400, Tejun Heo wrote:
> >
> > Maybe add that this led to a deadlock and add a Link tag to this
> > thread?
> 
> I'll add a note about the deadlock but I don't like Link tags
> because websites die and you can always just google the patch
> subject.

That's why we use http://lkml.kernel.org/r/MSG_ID links.

> > > +	nlk_sk(sk)->bound = !!portid;
> > 
> > !! isn't necessasry and this creates ordering between two stores.
> 
> !! was necessary because we're going from a u32 to a bool.

bool casting actually collapses the source value to a boolean value.
No need for casting regardless of data type.

> @@ -1076,17 +1076,19 @@ static int netlink_insert(struct sock *sk, u32 portid)
>  	    unlikely(atomic_read(&table->hash.nelems) >= UINT_MAX))
>  		goto err;
>  
> -	nlk_sk(sk)->portid = portid;
> +	nlk_sk(sk)->rhash_portid = portid;
>  	sock_hold(sk);
>  
>  	err = __netlink_insert(table, sk);
>  	if (err) {
>  		if (err == -EEXIST)
>  			err = -EADDRINUSE;
> -		nlk_sk(sk)->portid = 0;
>  		sock_put(sk);
> +		goto err;
>  	}
>  
> +	nlk_sk(sk)->portid = portid;

So, this doesn't necessarily make the ordering problem go away.  The
hash lookup would be fine but imagine a code path like the following.

	rcu_read_lock();
	sock = rhash lookup(some port number);
	do some operation which may use sock->portid;
	rcu_read_unlock();

Now, that some operation may see 0 as the port number.  I don't think
you can avoid doing some type of memory barrier operations if you
wanna gate autobind w/o grabbing locks.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

Possible netlink autobind regression Tejun Heo <tj@kernel.org> - 2015-09-17 04:30 +0200
  Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 05:10 +0200
    Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 05:50 +0200
      Re: Possible netlink autobind regression Cong Wang <cwang@twopensource.com> - 2015-09-17 07:10 +0200
        Re: Possible netlink autobind regression Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-17 07:20 +0200
          Re: Possible netlink autobind regression Tejun Heo <tj@kernel.org> - 2015-09-17 13:40 +0200
            [PATCH v3] netlink: Fix autobind race condition that leads to zero  port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-18 08:40 +0200
              [PATCH v4] netlink: Fix autobind race condition that leads to zero  port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-18 13:20 +0200
                Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID David Miller <davem@davemloft.net> - 2015-09-21 08:00 +0200
                Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 08:10 +0200
                Re: [PATCH v4] netlink: Fix autobind race condition that leads to  zero port ID David Miller <davem@davemloft.net> - 2015-09-21 08:20 +0200
                netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-21 15:40 +0200
                Re: netlink: Replace rhash_portid with bound Tejun Heo <tj@kernel.org> - 2015-09-21 20:30 +0200
                [PATCH v2] netlink: Replace rhash_portid with bound Herbert Xu <herbert@gondor.apana.org.au> - 2015-09-22 05:40 +0200
                [PATCH] netlink: Replace rhash_portid with load_acquire protected  boolean Tejun Heo <tj@kernel.org> - 2015-09-21 23:00 +0200
              Re: [PATCH v3] netlink: Fix autobind race condition that leads to  zero port ID Tejun Heo <tj@kernel.org> - 2015-09-18 15:40 +0200
          Re: Possible netlink autobind regression Thomas Graf <tgraf@suug.ch> - 2015-09-17 13:50 +0200

csiph-web