Re: [PATCH net v2] tipc: protect received keys from concurrent flush

[Jérémy Jean]

Hello,

On 2026-10-05 04:14, Tung Quang Nguyen wrote:
> > 	/* Attach it to the crypto */
> > 	if (likely(!rc)) {
> > -		rc = tipc_crypto_key_attach(c, aead, 0, master_key);
> > +		spin_lock_bh(&c->lock);
> > +		if (ukey == c->skey_in_use && c->skey != ukey)
> > +			rc = -ECANCELED;
>
> This check should be done earlier, before calling tipc_ahead_init() ?

Would moving the check before tipc_aead_init() be enough, given that a
flush can still happen while AEAD setup is running and before the key
is attached?

Indeed, the race goes something like:
1. Worker starts tipc_aead_init()
2. Flush clears rx->skey
3. Worker finishes AEAD setup

AFAIU, we still need the check under c->lock, immediately before
tipc_crypto_key_attach(), so that a revoked key cannot be published
after flush.

> > +		else
> > +			rc = tipc_crypto_key_attach(c, aead, 0, master_key);
> > +		spin_unlock_bh(&c->lock);
> > 		if (rc < 0)
> > 			tipc_aead_free(&aead->rcu);
> > 	}
> > @@ -1151,6 +1158,8 @@ int tipc_crypto_key_init(struct tipc_crypto *c, 
> > struct
> > tipc_aead_key *ukey,
> >  * @pos: desired slot in the crypto key array, = 0 if any!
> >  * @master_key: specify this is a cluster master key
> >  *
> > + * The caller must hold c->lock.
> > + *
> >  * Return: new key id in case of success, otherwise: -EBUSY
> >  */
> > static int tipc_crypto_key_attach(struct tipc_crypto *c, @@ -1158,20 
> > +1167,19
> > @@ static int tipc_crypto_key_attach(struct tipc_crypto *c,
> > 				  bool master_key)
> > {
> > 	struct tipc_key key;
> > -	int rc = -EBUSY;
> > 	u8 new_key;
> >
> > -	spin_lock_bh(&c->lock);
> > +	lockdep_assert_held(&c->lock);
> > 	key = c->key;
> > 	if (master_key) {
> > 		new_key = KEY_MASTER;
> > 		goto attach;
> > 	}
> > 	if (key.active && key.passive)
> > -		goto exit;
> > +		return -EBUSY;
> > 	if (key.pending) {
> > 		if (tipc_aead_users(c->aead[key.pending]) > 0)
> > -			goto exit;
> > +			return -EBUSY;
> > 		/* if (pos): ok with replacing, will be aligned when needed */
> > 		/* Replace it */
> > 		new_key = key.pending;
> > @@ -1201,11 +1209,7 @@ attach:
> > 	c->working = 1;
> > 	c->nokey = 0;
> > 	c->key_master |= master_key;
> > -	rc = new_key;
> > -
> > -exit:
> > -	spin_unlock_bh(&c->lock);
> > -	return rc;
> > +	return new_key;
>
> Remove lock from tipc_crypto_key_attach() is enough. Why adding many 
> irrelevant changes that do not seem necessary ?

Those extra changes in tipc_crypto_key_attach() are more cleanup than
anything else; I preferred the direct returns since the function no
longer owns the lock, but I can reduce the patch and keep the exit
label.

Thanks,
Jérémy