Re: [PATCH nf-next v3 09/16] netfilter: nfnetlink_cttimeout: use rcu protection in cttimeout_get_timeout

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Wed, Mar 23, 2022 at 02:22:07PM +0100, Florian Westphal wrote:
> I'd like to be able to switch lifetime management of ctnl_timeout
> to free-on-zero-refcount.
> 
> This isn't possible at the moment because removal of the structures
> from the pernet list requires the nfnl mutex and release may happen from
> softirq.
> 
> Current solution is to prevent this by disallowing policy object removal
> if the refcount is > 1 (i.e., policy is still referenced from the ruleset).
> 
> Switch traversal to rcu-read-lock as a first step to reduce reliance on
> nfnl mutex protection: removal from softirq would require a extra list
> spinlock.

Needs .type = NFNL_CB_RCU?

> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  net/netfilter/nfnetlink_cttimeout.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
> index eea486f32971..aef2547bb579 100644
> --- a/net/netfilter/nfnetlink_cttimeout.c
> +++ b/net/netfilter/nfnetlink_cttimeout.c
> @@ -253,6 +253,7 @@ static int cttimeout_get_timeout(struct sk_buff *skb,
>  				 const struct nlattr * const cda[])
>  {
>  	struct nfct_timeout_pernet *pernet = nfct_timeout_pernet(info->net);
> +	struct sk_buff *skb2;
>  	int ret = -ENOENT;
>  	char *name;
>  	struct ctnl_timeout *cur;
> @@ -268,31 +269,31 @@ static int cttimeout_get_timeout(struct sk_buff *skb,
>  		return -EINVAL;
>  	name = nla_data(cda[CTA_TIMEOUT_NAME]);
>  
> -	list_for_each_entry(cur, &pernet->nfct_timeout_list, head) {
> -		struct sk_buff *skb2;
> +	skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!skb2)
> +		return -ENOMEM;
> +
> +	rcu_read_lock();
>  
> +	list_for_each_entry_rcu(cur, &pernet->nfct_timeout_list, head) {
>  		if (strncmp(cur->name, name, CTNL_TIMEOUT_NAME_MAX) != 0)
>  			continue;
>  
> -		skb2 = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> -		if (skb2 == NULL) {
> -			ret = -ENOMEM;
> -			break;
> -		}
> -
>  		ret = ctnl_timeout_fill_info(skb2, NETLINK_CB(skb).portid,
>  					     info->nlh->nlmsg_seq,
>  					     NFNL_MSG_TYPE(info->nlh->nlmsg_type),
>  					     IPCTNL_MSG_TIMEOUT_NEW, cur);
> -		if (ret <= 0) {
> -			kfree_skb(skb2);
> +		if (ret <= 0)
>  			break;
> -		}
>  
> -		ret = nfnetlink_unicast(skb2, info->net, NETLINK_CB(skb).portid);
> -		break;
> +		rcu_read_unlock();
> +
> +		return nfnetlink_unicast(skb2, info->net, NETLINK_CB(skb).portid);
>  	}
>  
> +	rcu_read_unlock();
> +	kfree_skb(skb2);
> +
>  	return ret;
>  }
>  
> -- 
> 2.34.1
>