Re: [PATCH net-next v15 09/22] ovpn: implement packet processing

[Sabrina Dubroca <sd@queasysnail.net>]

(just a few nits here)

2024-12-11, 22:15:13 +0100, Antonio Quartulli wrote:
> +static inline struct ovpn_crypto_key_slot *
> +ovpn_crypto_key_id_to_slot(const struct ovpn_crypto_state *cs, u8 key_id)
> +{
> +	struct ovpn_crypto_key_slot *ks;
> +	u8 idx;
> +
> +	if (unlikely(!cs))
> +		return NULL;
> +
> +	rcu_read_lock();
> +	idx = cs->primary_idx;
> +	ks = rcu_dereference(cs->slots[idx]);
> +	if (ks && ks->key_id == key_id) {
> +		if (unlikely(!ovpn_crypto_key_slot_hold(ks)))
> +			ks = NULL;
> +		goto out;
> +	}
> +
> +	ks = rcu_dereference(cs->slots[idx ^ 1]);

nit: for consistency with the other uses of the secondary slot, I'd
switch that to cs->slots[!idx]

[...]
> +int ovpn_aead_encrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
> +		      struct sk_buff *skb)
> +{
[...]
> +	/* add packet op as head of additional data */
> +	op = ovpn_opcode_compose(OVPN_DATA_V2, ks->key_id, peer->id);
> +	__skb_push(skb, OVPN_OPCODE_SIZE);
> +	BUILD_BUG_ON(sizeof(op) != OVPN_OPCODE_SIZE);
> +	*((__force __be32 *)skb->data) = htonl(op);
> +
> +	/* AEAD Additional data */
> +	sg_set_buf(sg, skb->data, OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE);

You could add

#define OVPN_AAD_SIZE (OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE)

and then use it in a few places in ovpn_aead_encrypt and
ovpn_aead_decrypt.


[...]
> +int ovpn_aead_decrypt(struct ovpn_peer *peer, struct ovpn_crypto_key_slot *ks,
> +		      struct sk_buff *skb)
> +{
> +	const unsigned int tag_size = crypto_aead_authsize(ks->decrypt);
> +	int ret, payload_len, nfrags;
> +	unsigned int payload_offset;
> +	struct aead_request *req;
> +	struct sk_buff *trailer;
> +	struct scatterlist *sg;
> +	u8 iv[OVPN_NONCE_SIZE];
> +	unsigned int sg_len;
> +
> +	payload_offset = OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE + tag_size;

OVPN_AAD_SIZE + tag_size


[...]
> +	sg_init_table(sg, nfrags + 2);
> +
> +	/* packet op is head of additional data */
> +	sg_len = OVPN_OPCODE_SIZE + OVPN_NONCE_WIRE_SIZE;

This variable can probably be dropped if you add OVPN_AAD_SIZE.

[...]
> +	/* setup async crypto operation */
> +	aead_request_set_tfm(req, ks->decrypt);
> +	aead_request_set_callback(req, 0, ovpn_decrypt_post, skb);
> +	aead_request_set_crypt(req, sg, sg, payload_len + tag_size, iv);
> +
> +	aead_request_set_ad(req, OVPN_NONCE_WIRE_SIZE + OVPN_OPCODE_SIZE);

and this op is flipped but it's still OVPN_AAD_SIZE.

> +
> +	/* decrypt it */
> +	return crypto_aead_decrypt(req);
> +free_sg:
> +	kfree(ovpn_skb_cb(skb)->sg);
> +	ovpn_skb_cb(skb)->sg = NULL;
> +	return ret;
> +}
> +

[...]
> @@ -80,7 +83,10 @@ static void ovpn_peer_release_rcu(struct rcu_head *head)
>   */
>  static void ovpn_peer_release(struct ovpn_peer *peer)
>  {
> +	ovpn_crypto_state_release(&peer->crypto);
> +	spin_lock_bh(&peer->lock);
>  	ovpn_bind_reset(peer, NULL);

At this point in the series, ovpn_bind_reset also tries to take
peer->lock (gets fixed in the "peer floating" patch).

(but if you're tired of moving chunks around, I can live with it)

> +	spin_unlock_bh(&peer->lock);
>  	netdev_put(peer->ovpn->dev, &peer->ovpn->dev_tracker);
>  	call_rcu(&peer->rcu, ovpn_peer_release_rcu);
>  }

-- 
Sabrina