| Message ID | 20220323132214.6700-10-fw@strlen.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | netfilter: conntrack: remove percpu lists | expand |
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 >
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; }
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. Signed-off-by: Florian Westphal <fw@strlen.de> --- net/netfilter/nfnetlink_cttimeout.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)