Skip to content

Commit

Permalink
mctp: locking, lifetime and validity changes for sk_keys
Browse files Browse the repository at this point in the history 
We will want to invalidate sk_keys in a future change, which will
require a boolean flag to mark invalidated items in the socket & net
namespace lists. We'll also need to take a reference to keys, held over
non-atomic contexts, so we need a refcount on keys also.

This change adds a validity flag (currently always true) and refcount to
struct mctp_sk_key.  With a refcount on the keys, using RCU no longer
makes much sense; we have exact indications on the lifetime of keys. So,
we also change the RCU list traversal to a locked implementation.

Signed-off-by: Jeremy Kerr <jk@codeconstruct.com.au>
Signed-off-by: David S. Miller <davem@davemloft.net>
  • Loading branch information
Jeremy Kerr authored and David S. Miller committed Sep 29, 2021
1 parent 1f6c77a commit 73c6184
Show file tree
Hide file tree
Showing 3 changed files with 125 additions and 53 deletions.
46 changes: 31 additions & 15 deletions include/net/mctp.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,30 +67,36 @@ struct mctp_sock {
/* Key for matching incoming packets to sockets or reassembly contexts.
* Packets are matched on (src,dest,tag).
*
* Lifetime requirements:
* Lifetime / locking requirements:
*
* - keys are free()ed via RCU
* - individual key data (ie, the struct itself) is protected by key->lock;
* changes must be made with that lock held.
*
* - the lookup fields: peer_addr, local_addr and tag are set before the
* key is added to lookup lists, and never updated.
*
* - A ref to the key must be held (throuh key->refs) if a pointer to the
* key is to be accessed after key->lock is released.
*
* - a mctp_sk_key contains a reference to a struct sock; this is valid
* for the life of the key. On sock destruction (through unhash), the key is
* removed from lists (see below), and will not be observable after a RCU
* grace period.
*
* any RX occurring within that grace period may still queue to the socket,
* but will hit the SOCK_DEAD case before the socket is freed.
* removed from lists (see below), and marked invalid.
*
* - these mctp_sk_keys appear on two lists:
* 1) the struct mctp_sock->keys list
* 2) the struct netns_mctp->keys list
*
* updates to either list are performed under the netns_mctp->keys
* lock.
* presences on these lists requires a (single) refcount to be held; both
* lists are updated as a single operation.
*
* Updates and lookups in either list are performed under the
* netns_mctp->keys lock. Lookup functions will need to lock the key and
* take a reference before unlocking the keys_lock. Consequently, the list's
* keys_lock *cannot* be acquired with the individual key->lock held.
*
* - a key may have a sk_buff attached as part of an in-progress message
* reassembly (->reasm_head). The reassembly context is protected by
* reasm_lock, which may be acquired with the keys lock (above) held, if
* necessary. Consequently, keys lock *cannot* be acquired with the
* reasm_lock held.
* reassembly (->reasm_head). The reasm data is protected by the individual
* key->lock.
*
* - there are two destruction paths for a mctp_sk_key:
*
Expand All @@ -116,14 +122,22 @@ struct mctp_sk_key {
/* per-socket list */
struct hlist_node sklist;

/* lock protects against concurrent updates to the reassembly and
* expiry data below.
*/
spinlock_t lock;

/* Keys are referenced during the output path, which may sleep */
refcount_t refs;

/* incoming fragment reassembly context */
spinlock_t reasm_lock;
struct sk_buff *reasm_head;
struct sk_buff **reasm_tailp;
bool reasm_dead;
u8 last_seq;

struct rcu_head rcu;
/* key validity */
bool valid;
};

struct mctp_skb_cb {
Expand Down Expand Up @@ -191,6 +205,8 @@ int mctp_do_route(struct mctp_route *rt, struct sk_buff *skb);
int mctp_local_output(struct sock *sk, struct mctp_route *rt,
struct sk_buff *skb, mctp_eid_t daddr, u8 req_tag);

void mctp_key_unref(struct mctp_sk_key *key);

/* routing <--> device interface */
unsigned int mctp_default_net(struct net *net);
int mctp_default_net_set(struct net *net, unsigned int index);
Expand Down
14 changes: 7 additions & 7 deletions net/mctp/af_mctp.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,21 +263,21 @@ static void mctp_sk_unhash(struct sock *sk)
/* remove tag allocations */
spin_lock_irqsave(&net->mctp.keys_lock, flags);
hlist_for_each_entry_safe(key, tmp, &msk->keys, sklist) {
hlist_del_rcu(&key->sklist);
hlist_del_rcu(&key->hlist);
hlist_del(&key->sklist);
hlist_del(&key->hlist);

spin_lock(&key->reasm_lock);
spin_lock(&key->lock);
if (key->reasm_head)
kfree_skb(key->reasm_head);
key->reasm_head = NULL;
key->reasm_dead = true;
spin_unlock(&key->reasm_lock);
key->valid = false;
spin_unlock(&key->lock);

kfree_rcu(key, rcu);
/* key is no longer on the lookup lists, unref */
mctp_key_unref(key);
}
spin_unlock_irqrestore(&net->mctp.keys_lock, flags);

synchronize_rcu();
}

static struct proto mctp_proto = {
Expand Down
118 changes: 87 additions & 31 deletions net/mctp/route.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,25 +83,43 @@ static bool mctp_key_match(struct mctp_sk_key *key, mctp_eid_t local,
return true;
}

/* returns a key (with key->lock held, and refcounted), or NULL if no such
* key exists.
*/
static struct mctp_sk_key *mctp_lookup_key(struct net *net, struct sk_buff *skb,
mctp_eid_t peer)
mctp_eid_t peer,
unsigned long *irqflags)
__acquires(&key->lock)
{
struct mctp_sk_key *key, *ret;
unsigned long flags;
struct mctp_hdr *mh;
u8 tag;

WARN_ON(!rcu_read_lock_held());

mh = mctp_hdr(skb);
tag = mh->flags_seq_tag & (MCTP_HDR_TAG_MASK | MCTP_HDR_FLAG_TO);

ret = NULL;
spin_lock_irqsave(&net->mctp.keys_lock, flags);

hlist_for_each_entry_rcu(key, &net->mctp.keys, hlist) {
if (mctp_key_match(key, mh->dest, peer, tag)) {
hlist_for_each_entry(key, &net->mctp.keys, hlist) {
if (!mctp_key_match(key, mh->dest, peer, tag))
continue;

spin_lock(&key->lock);
if (key->valid) {
refcount_inc(&key->refs);
ret = key;
break;
}
spin_unlock(&key->lock);
}

if (ret) {
spin_unlock(&net->mctp.keys_lock);
*irqflags = flags;
} else {
spin_unlock_irqrestore(&net->mctp.keys_lock, flags);
}

return ret;
Expand All @@ -121,11 +139,19 @@ static struct mctp_sk_key *mctp_key_alloc(struct mctp_sock *msk,
key->local_addr = local;
key->tag = tag;
key->sk = &msk->sk;
spin_lock_init(&key->reasm_lock);
key->valid = true;
spin_lock_init(&key->lock);
refcount_set(&key->refs, 1);

return key;
}

void mctp_key_unref(struct mctp_sk_key *key)
{
if (refcount_dec_and_test(&key->refs))
kfree(key);
}

static int mctp_key_add(struct mctp_sk_key *key, struct mctp_sock *msk)
{
struct net *net = sock_net(&msk->sk);
Expand All @@ -138,12 +164,17 @@ static int mctp_key_add(struct mctp_sk_key *key, struct mctp_sock *msk)
hlist_for_each_entry(tmp, &net->mctp.keys, hlist) {
if (mctp_key_match(tmp, key->local_addr, key->peer_addr,
key->tag)) {
rc = -EEXIST;
break;
spin_lock(&tmp->lock);
if (tmp->valid)
rc = -EEXIST;
spin_unlock(&tmp->lock);
if (rc)
break;
}
}

if (!rc) {
refcount_inc(&key->refs);
hlist_add_head(&key->hlist, &net->mctp.keys);
hlist_add_head(&key->sklist, &msk->keys);
}
Expand All @@ -153,28 +184,35 @@ static int mctp_key_add(struct mctp_sk_key *key, struct mctp_sock *msk)
return rc;
}

/* Must be called with key->reasm_lock, which it will release. Will schedule
* the key for an RCU free.
/* We're done with the key; unset valid and remove from lists. There may still
* be outstanding refs on the key though...
*/
static void __mctp_key_unlock_drop(struct mctp_sk_key *key, struct net *net,
unsigned long flags)
__releases(&key->reasm_lock)
__releases(&key->lock)
{
struct sk_buff *skb;

skb = key->reasm_head;
key->reasm_head = NULL;
key->reasm_dead = true;
spin_unlock_irqrestore(&key->reasm_lock, flags);
key->valid = false;
spin_unlock_irqrestore(&key->lock, flags);

spin_lock_irqsave(&net->mctp.keys_lock, flags);
hlist_del_rcu(&key->hlist);
hlist_del_rcu(&key->sklist);
hlist_del(&key->hlist);
hlist_del(&key->sklist);
spin_unlock_irqrestore(&net->mctp.keys_lock, flags);
kfree_rcu(key, rcu);

/* one unref for the lists */
mctp_key_unref(key);

/* and one for the local reference */
mctp_key_unref(key);

if (skb)
kfree_skb(skb);

}

static int mctp_frag_queue(struct mctp_sk_key *key, struct sk_buff *skb)
Expand Down Expand Up @@ -248,8 +286,10 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)

rcu_read_lock();

/* lookup socket / reasm context, exactly matching (src,dest,tag) */
key = mctp_lookup_key(net, skb, mh->src);
/* lookup socket / reasm context, exactly matching (src,dest,tag).
* we hold a ref on the key, and key->lock held.
*/
key = mctp_lookup_key(net, skb, mh->src, &f);

if (flags & MCTP_HDR_FLAG_SOM) {
if (key) {
Expand All @@ -260,10 +300,12 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
* key for reassembly - we'll create a more specific
* one for future packets if required (ie, !EOM).
*/
key = mctp_lookup_key(net, skb, MCTP_ADDR_ANY);
key = mctp_lookup_key(net, skb, MCTP_ADDR_ANY, &f);
if (key) {
msk = container_of(key->sk,
struct mctp_sock, sk);
spin_unlock_irqrestore(&key->lock, f);
mctp_key_unref(key);
key = NULL;
}
}
Expand All @@ -282,11 +324,11 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
if (flags & MCTP_HDR_FLAG_EOM) {
sock_queue_rcv_skb(&msk->sk, skb);
if (key) {
spin_lock_irqsave(&key->reasm_lock, f);
/* we've hit a pending reassembly; not much we
* can do but drop it
*/
__mctp_key_unlock_drop(key, net, f);
key = NULL;
}
rc = 0;
goto out_unlock;
Expand All @@ -303,7 +345,7 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
goto out_unlock;
}

/* we can queue without the reasm lock here, as the
/* we can queue without the key lock here, as the
* key isn't observable yet
*/
mctp_frag_queue(key, skb);
Expand All @@ -318,17 +360,17 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
if (rc)
kfree(key);

} else {
/* existing key: start reassembly */
spin_lock_irqsave(&key->reasm_lock, f);
/* we don't need to release key->lock on exit */
key = NULL;

} else {
if (key->reasm_head || key->reasm_dead) {
/* duplicate start? drop everything */
__mctp_key_unlock_drop(key, net, f);
rc = -EEXIST;
key = NULL;
} else {
rc = mctp_frag_queue(key, skb);
spin_unlock_irqrestore(&key->reasm_lock, f);
}
}

Expand All @@ -337,8 +379,6 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
* using the message-specific key
*/

spin_lock_irqsave(&key->reasm_lock, f);

/* we need to be continuing an existing reassembly... */
if (!key->reasm_head)
rc = -EINVAL;
Expand All @@ -352,8 +392,7 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)
sock_queue_rcv_skb(key->sk, key->reasm_head);
key->reasm_head = NULL;
__mctp_key_unlock_drop(key, net, f);
} else {
spin_unlock_irqrestore(&key->reasm_lock, f);
key = NULL;
}

} else {
Expand All @@ -363,6 +402,10 @@ static int mctp_route_input(struct mctp_route *route, struct sk_buff *skb)

out_unlock:
rcu_read_unlock();
if (key) {
spin_unlock_irqrestore(&key->lock, f);
mctp_key_unref(key);
}
out:
if (rc)
kfree_skb(skb);
Expand Down Expand Up @@ -459,6 +502,7 @@ static void mctp_reserve_tag(struct net *net, struct mctp_sk_key *key,
*/
hlist_add_head_rcu(&key->hlist, &mns->keys);
hlist_add_head_rcu(&key->sklist, &msk->keys);
refcount_inc(&key->refs);
}

/* Allocate a locally-owned tag value for (saddr, daddr), and reserve
Expand Down Expand Up @@ -492,14 +536,26 @@ static int mctp_alloc_local_tag(struct mctp_sock *msk,
* tags. If we find a conflict, clear that bit from tagbits
*/
hlist_for_each_entry(tmp, &mns->keys, hlist) {
/* We can check the lookup fields (*_addr, tag) without the
* lock held, they don't change over the lifetime of the key.
*/

/* if we don't own the tag, it can't conflict */
if (tmp->tag & MCTP_HDR_FLAG_TO)
continue;

if ((tmp->peer_addr == daddr ||
tmp->peer_addr == MCTP_ADDR_ANY) &&
tmp->local_addr == saddr)
if (!((tmp->peer_addr == daddr ||
tmp->peer_addr == MCTP_ADDR_ANY) &&
tmp->local_addr == saddr))
continue;

spin_lock(&tmp->lock);
/* key must still be valid. If we find a match, clear the
* potential tag value
*/
if (tmp->valid)
tagbits &= ~(1 << tmp->tag);
spin_unlock(&tmp->lock);

if (!tagbits)
break;
Expand Down

0 comments on commit 73c6184

Please sign in to comment.