On (12/30/17 21:09), santosh.shilim...@oracle.com wrote:
> Right. This was loop transport in action so xmit will just flip
> the direction with receive. And rds_recv_incoming() can race with
> socket_release. rds_find_bound() is suppose to add ref count on
> socket for rds_recv_incoming() but by that time socket is DEAD &
> freed by socket release callback.
correct, that makes sense.
> And rds_release is suppose to be synced with rs_recv_lock. But
> release callback is marking the sk orphan before syncing
> up with receive path and updating the bind table. Probably it
> can pushed down further after the socket clean up buut need
> to think bit more.
However, I'm not sure this seals the race.. according to the
bug report rds_recv_incoming->rds_find_bound is being called
in rds_send_worker context and the rds_find_bound code is
63 rs = rhashtable_lookup_fast(&bind_hash_table, &key, ht_parms);
64 if (rs && !sock_flag(rds_rs_to_sk(rs), SOCK_DEAD))
65 rds_sock_addref(rs);
66 else
67 rs = NULL;
68
Since the entire logic of rds_release can interleave between line 63
and 64, (whereas we only addref at line 65), moving the sock_orphan
will not help.
I see that there was an explicic synchornization via the bucket->lock
before 7b5654349e. I think you need something like that, or some type
or rcu-based hash list.
Patch below may make race-window smaller, but race window is still there.
>
>
> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index b405f77..11e1426 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -65,7 +65,6 @@ static int rds_release(struct socket *sock)
>
> rs = rds_sk_to_rs(sk);
>
> - sock_orphan(sk);
> /* Note - rds_clear_recv_queue grabs rs_recv_lock, so
> * that ensures the recv path has completed messing
> * with the socket. */
> @@ -85,6 +84,7 @@ static int rds_release(struct socket *sock)
>
> rds_trans_put(rs->rs_transport);
>
> + sock_orphan(sk);
> sock->sk = NULL;
> sock_put(sk);
> out:
