| Message ID | 20171128021309.11277-2-pablo@netfilter.org |
|---|---|
| State | RFC |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next,RFC,1/3] netfilter: nf_conntrack: add 64-bit conntrack ID extension | expand |
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > static int ctnetlink_flush_conntrack(struct net *net, > @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl, > nf_ct_put(ct); > return -ENOENT; > } > + } else if (cda[CTA_ID64]) { > + u64 id = ntohl(nla_get_be64(cda[CTA_ID64])); be64_to_cpu()? But at this point we already uniquely identified the conntrack entry so the ID check appears to be unneeded? I never understood existing test either, so this remark isn't specific to your patch. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 28, 2017 at 01:12:06PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > static int ctnetlink_flush_conntrack(struct net *net, > > @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl, > > nf_ct_put(ct); > > return -ENOENT; > > } > > + } else if (cda[CTA_ID64]) { > > + u64 id = ntohl(nla_get_be64(cda[CTA_ID64])); > > be64_to_cpu()? > > But at this point we already uniquely identified the conntrack entry > so the ID check appears to be unneeded? > > I never understood existing test either, so this remark isn't specific > to your patch. When the ID was incremental, not a memory address, you could use it to specifically refer to a conntrack through tuple + id. If a conntrack with tuple X is gone, then created again, you refer to the right object. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Nov 28, 2017 at 01:12:06PM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > static int ctnetlink_flush_conntrack(struct net *net, > > > @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl, > > > nf_ct_put(ct); > > > return -ENOENT; > > > } > > > + } else if (cda[CTA_ID64]) { > > > + u64 id = ntohl(nla_get_be64(cda[CTA_ID64])); > > > > be64_to_cpu()? > > > > But at this point we already uniquely identified the conntrack entry > > so the ID check appears to be unneeded? > > > > I never understood existing test either, so this remark isn't specific > > to your patch. > > When the ID was incremental, not a memory address, you could use it to > specifically refer to a conntrack through tuple + id. > > If a conntrack with tuple X is gone, then created again, you refer to > the right object. Yes, but why was that needed?! I understand what it does, I don't understand the use case :) -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/uapi/linux/netfilter/nfnetlink_conntrack.h b/include/uapi/linux/netfilter/nfnetlink_conntrack.h index 7397e022ce6e..dcd7b97eeeac 100644 --- a/include/uapi/linux/netfilter/nfnetlink_conntrack.h +++ b/include/uapi/linux/netfilter/nfnetlink_conntrack.h @@ -54,6 +54,8 @@ enum ctattr_type { CTA_MARK_MASK, CTA_LABELS, CTA_LABELS_MASK, + CTA_ID64, + CTA_PAD, __CTA_MAX }; #define CTA_MAX (__CTA_MAX - 1) diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c index b3b8249ced4a..7aecb8ae5ecc 100644 --- a/net/netfilter/nf_conntrack_netlink.c +++ b/net/netfilter/nf_conntrack_netlink.c @@ -446,7 +446,8 @@ static int ctnetlink_dump_ct_seq_adj(struct sk_buff *skb, struct nf_conn *ct) static int ctnetlink_dump_id(struct sk_buff *skb, const struct nf_conn *ct) { - if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct))) + if (nla_put_be32(skb, CTA_ID, htonl((unsigned long)ct)) || + nla_put_be64(skb, CTA_ID64, cpu_to_be64(nf_ct_id(ct)), CTA_PAD)) goto nla_put_failure; return 0; @@ -600,6 +601,7 @@ static size_t ctnetlink_nlmsg_size(const struct nf_conn *ct) + 3 * nla_total_size(0) /* CTA_TUPLE_PROTO */ + 3 * nla_total_size(sizeof(u_int8_t)) /* CTA_PROTO_NUM */ + nla_total_size(sizeof(u_int32_t)) /* CTA_ID */ + + nla_total_size(sizeof(u64)) /* CTA_ID64 */ + nla_total_size(sizeof(u_int32_t)) /* CTA_STATUS */ + ctnetlink_acct_size(ct) + ctnetlink_timestamp_size(ct) @@ -1108,6 +1110,7 @@ static const struct nla_policy ct_nla_policy[CTA_MAX+1] = { .len = NF_CT_LABELS_MAX_SIZE }, [CTA_LABELS_MASK] = { .type = NLA_BINARY, .len = NF_CT_LABELS_MAX_SIZE }, + [CTA_ID64] = { .type = NLA_U64 }, }; static int ctnetlink_flush_conntrack(struct net *net, @@ -1174,6 +1177,13 @@ static int ctnetlink_del_conntrack(struct net *net, struct sock *ctnl, nf_ct_put(ct); return -ENOENT; } + } else if (cda[CTA_ID64]) { + u64 id = ntohl(nla_get_be64(cda[CTA_ID64])); + + if (id != nf_ct_id(ct)) { + nf_ct_put(ct); + return -ENOENT; + } } nf_ct_delete(ct, NETLINK_CB(skb).portid, nlmsg_report(nlh)); @@ -1319,7 +1329,7 @@ ctnetlink_dump_list(struct sk_buff *skb, struct netlink_callback *cb, bool dying if (!atomic_inc_not_zero(&ct->ct_general.use)) continue; cb->args[0] = cpu; - cb->args[1] = (unsigned long)ct; + cb->args[1] = nf_ct_id(ct); spin_unlock_bh(&pcpu->lock); goto out; }
The older 32-bit conntrack ID is still exposed for backward compatibility reasons, add new CTA_ID64 attribute. Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- include/uapi/linux/netfilter/nfnetlink_conntrack.h | 2 ++ net/netfilter/nf_conntrack_netlink.c | 14 ++++++++++++-- 2 files changed, 14 insertions(+), 2 deletions(-)