netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
@ 2024-10-02 15:55 Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 1/4] netfilter: xt_nat: compact nf_nat_setup_info calls Florian Westphal
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-02 15:55 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

Provide more precise drop information rather than doing the freeing
from core.c:nf_hook_slow().

First patch is a small preparation patch, rest coverts NF_DROP
locations of NF_DROP_REASON().

Florian Westphal (4):
  netfilter: xt_nat: compact nf_nat_setup_info calls
  netfilter: xt_nat: drop packet earlier
  netfilter: nf_nat: use skb_drop_reason
  netfilter: nf_tables: use skb_drop_reason

 include/linux/netfilter.h                 |  5 +-
 net/bridge/netfilter/nft_reject_bridge.c  |  2 +-
 net/ipv4/netfilter/nft_reject_ipv4.c      |  2 +-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c |  5 +-
 net/netfilter/nf_nat_masquerade.c         | 23 +++++--
 net/netfilter/nf_nat_proto.c              | 18 +++---
 net/netfilter/nf_nat_redirect.c           |  4 +-
 net/netfilter/nf_synproxy_core.c          | 16 ++---
 net/netfilter/nft_chain_filter.c          |  4 +-
 net/netfilter/nft_compat.c                |  8 +--
 net/netfilter/nft_connlimit.c             |  4 +-
 net/netfilter/nft_ct.c                    | 14 ++--
 net/netfilter/nft_exthdr.c                |  2 +-
 net/netfilter/nft_fib_inet.c              |  2 +-
 net/netfilter/nft_fwd_netdev.c            |  4 +-
 net/netfilter/nft_nat.c                   |  8 ++-
 net/netfilter/nft_reject_inet.c           |  2 +-
 net/netfilter/nft_reject_netdev.c         |  2 +-
 net/netfilter/nft_synproxy.c              | 10 +--
 net/netfilter/xt_nat.c                    | 78 ++++++++++-------------
 20 files changed, 112 insertions(+), 101 deletions(-)

-- 
2.45.2


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH nf-next 1/4] netfilter: xt_nat: compact nf_nat_setup_info calls
  2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
@ 2024-10-02 15:55 ` Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 2/4] netfilter: xt_nat: drop packet earlier Florian Westphal
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-02 15:55 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

Collapse some common code, this will allow to use
kfree_skb_drop_reason in the next patch.

No semantic change intended.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/xt_nat.c | 73 ++++++++++++++++--------------------------
 1 file changed, 28 insertions(+), 45 deletions(-)

diff --git a/net/netfilter/xt_nat.c b/net/netfilter/xt_nat.c
index b4f7bbc3f3ca..d04f7cf6b94d 100644
--- a/net/netfilter/xt_nat.c
+++ b/net/netfilter/xt_nat.c
@@ -13,6 +13,26 @@
 #include <linux/netfilter/x_tables.h>
 #include <net/netfilter/nf_nat.h>
 
+static unsigned int
+xt_nat_setup_info(struct sk_buff *skb,
+		  const struct nf_nat_range2 *range,
+		  enum nf_nat_manip_type maniptype)
+{
+	enum ip_conntrack_info ctinfo;
+	struct nf_conn *ct;
+
+	ct = nf_ct_get(skb, &ctinfo);
+	if (WARN_ON(!ct))
+		return NF_ACCEPT;
+
+	if (WARN_ON(!(ctinfo == IP_CT_NEW ||
+		      ctinfo == IP_CT_RELATED ||
+	    (ctinfo == IP_CT_RELATED_REPLY && maniptype == NF_NAT_MANIP_SRC))))
+		return NF_ACCEPT;
+
+	return nf_nat_setup_info(ct, range, maniptype);
+}
+
 static int xt_nat_checkentry_v0(const struct xt_tgchk_param *par)
 {
 	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
@@ -53,16 +73,10 @@ xt_snat_target_v0(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
 	struct nf_nat_range2 range;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
-		  ctinfo == IP_CT_RELATED_REPLY)));
 
 	xt_nat_convert_range(&range, &mr->range[0]);
-	return nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
+
+	return xt_nat_setup_info(skb, &range, NF_NAT_MANIP_SRC);
 }
 
 static unsigned int
@@ -70,15 +84,10 @@ xt_dnat_target_v0(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_ipv4_multi_range_compat *mr = par->targinfo;
 	struct nf_nat_range2 range;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
 
 	xt_nat_convert_range(&range, &mr->range[0]);
-	return nf_nat_setup_info(ct, &range, NF_NAT_MANIP_DST);
+
+	return xt_nat_setup_info(skb, &range, NF_NAT_MANIP_DST);
 }
 
 static unsigned int
@@ -86,18 +95,11 @@ xt_snat_target_v1(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_range *range_v1 = par->targinfo;
 	struct nf_nat_range2 range;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
-		  ctinfo == IP_CT_RELATED_REPLY)));
 
 	memcpy(&range, range_v1, sizeof(*range_v1));
 	memset(&range.base_proto, 0, sizeof(range.base_proto));
 
-	return nf_nat_setup_info(ct, &range, NF_NAT_MANIP_SRC);
+	return xt_nat_setup_info(skb, &range, NF_NAT_MANIP_SRC);
 }
 
 static unsigned int
@@ -105,46 +107,27 @@ xt_dnat_target_v1(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_range *range_v1 = par->targinfo;
 	struct nf_nat_range2 range;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
 
 	memcpy(&range, range_v1, sizeof(*range_v1));
 	memset(&range.base_proto, 0, sizeof(range.base_proto));
 
-	return nf_nat_setup_info(ct, &range, NF_NAT_MANIP_DST);
+	return xt_nat_setup_info(skb, &range, NF_NAT_MANIP_DST);
 }
 
 static unsigned int
 xt_snat_target_v2(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_range2 *range = par->targinfo;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
-		  ctinfo == IP_CT_RELATED_REPLY)));
 
-	return nf_nat_setup_info(ct, range, NF_NAT_MANIP_SRC);
+	return xt_nat_setup_info(skb, range, NF_NAT_MANIP_SRC);
 }
 
 static unsigned int
 xt_dnat_target_v2(struct sk_buff *skb, const struct xt_action_param *par)
 {
 	const struct nf_nat_range2 *range = par->targinfo;
-	enum ip_conntrack_info ctinfo;
-	struct nf_conn *ct;
-
-	ct = nf_ct_get(skb, &ctinfo);
-	WARN_ON(!(ct != NULL &&
-		 (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED)));
 
-	return nf_nat_setup_info(ct, range, NF_NAT_MANIP_DST);
+	return xt_nat_setup_info(skb, range, NF_NAT_MANIP_DST);
 }
 
 static struct xt_target xt_nat_target_reg[] __read_mostly = {
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH nf-next 2/4] netfilter: xt_nat: drop packet earlier
  2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 1/4] netfilter: xt_nat: compact nf_nat_setup_info calls Florian Westphal
@ 2024-10-02 15:55 ` Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 3/4] netfilter: nf_nat: use skb_drop_reason Florian Westphal
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-02 15:55 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

Let net dropmonitor pick up a more specific location rather than the
catchall core.c:nf_hook_slow drop point.

This isn't moved into nf_nat_setup_info() because we do not pass
the skb to it.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/xt_nat.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/xt_nat.c b/net/netfilter/xt_nat.c
index d04f7cf6b94d..aaf31bd8381b 100644
--- a/net/netfilter/xt_nat.c
+++ b/net/netfilter/xt_nat.c
@@ -20,6 +20,7 @@ xt_nat_setup_info(struct sk_buff *skb,
 {
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct;
+	int ret;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	if (WARN_ON(!ct))
@@ -30,7 +31,11 @@ xt_nat_setup_info(struct sk_buff *skb,
 	    (ctinfo == IP_CT_RELATED_REPLY && maniptype == NF_NAT_MANIP_SRC))))
 		return NF_ACCEPT;
 
-	return nf_nat_setup_info(ct, range, maniptype);
+	ret = nf_nat_setup_info(ct, range, maniptype);
+	if (ret != NF_DROP)
+		return ret;
+
+	return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static int xt_nat_checkentry_v0(const struct xt_tgchk_param *par)
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH nf-next 3/4] netfilter: nf_nat: use skb_drop_reason
  2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 1/4] netfilter: xt_nat: compact nf_nat_setup_info calls Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 2/4] netfilter: xt_nat: drop packet earlier Florian Westphal
@ 2024-10-02 15:55 ` Florian Westphal
  2024-10-02 15:55 ` [PATCH nf-next 4/4] netfilter: nf_tables: " Florian Westphal
  2024-10-12 14:09 ` [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Pablo Neira Ayuso
  4 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-02 15:55 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

same as previous patch: extend nftables nat and masquerade functions to
indicate more precise drop locations.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/netfilter/nf_nat_masquerade.c | 23 ++++++++++++++++-------
 net/netfilter/nft_nat.c           |  8 +++++++-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_nat_masquerade.c b/net/netfilter/nf_nat_masquerade.c
index 1a506b0c6511..06f5968dbc15 100644
--- a/net/netfilter/nf_nat_masquerade.c
+++ b/net/netfilter/nf_nat_masquerade.c
@@ -35,6 +35,7 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
 	struct nf_nat_range2 newrange;
 	const struct rtable *rt;
 	__be32 newsrc, nh;
+	int ret;
 
 	WARN_ON(hooknum != NF_INET_POST_ROUTING);
 
@@ -52,10 +53,8 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
 	rt = skb_rtable(skb);
 	nh = rt_nexthop(rt, ip_hdr(skb)->daddr);
 	newsrc = inet_select_addr(out, nh, RT_SCOPE_UNIVERSE);
-	if (!newsrc) {
-		pr_info("%s ate my IP address\n", out->name);
-		return NF_DROP;
-	}
+	if (!newsrc)
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EADDRNOTAVAIL);
 
 	nat = nf_ct_nat_ext_add(ct);
 	if (nat)
@@ -71,7 +70,12 @@ nf_nat_masquerade_ipv4(struct sk_buff *skb, unsigned int hooknum,
 	newrange.max_proto   = range->max_proto;
 
 	/* Hand modified range to generic setup. */
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_SRC);
+	ret = nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_SRC);
+	if (ret == NF_DROP)
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP,
+				      EPERM);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nf_nat_masquerade_ipv4);
 
@@ -246,6 +250,7 @@ nf_nat_masquerade_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 	struct in6_addr src;
 	struct nf_conn *ct;
 	struct nf_nat_range2 newrange;
+	int ret;
 
 	ct = nf_ct_get(skb, &ctinfo);
 	WARN_ON(!(ct && (ctinfo == IP_CT_NEW || ctinfo == IP_CT_RELATED ||
@@ -253,7 +258,7 @@ nf_nat_masquerade_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 
 	if (nat_ipv6_dev_get_saddr(nf_ct_net(ct), out,
 				   &ipv6_hdr(skb)->daddr, 0, &src) < 0)
-		return NF_DROP;
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EADDRNOTAVAIL);
 
 	nat = nf_ct_nat_ext_add(ct);
 	if (nat)
@@ -265,7 +270,11 @@ nf_nat_masquerade_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 	newrange.min_proto	= range->min_proto;
 	newrange.max_proto	= range->max_proto;
 
-	return nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_SRC);
+	ret = nf_nat_setup_info(ct, &newrange, NF_NAT_MANIP_SRC);
+	if (ret == NF_DROP)
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(nf_nat_masquerade_ipv6);
 
diff --git a/net/netfilter/nft_nat.c b/net/netfilter/nft_nat.c
index 6e21f72c5b57..bd2bda5c2b13 100644
--- a/net/netfilter/nft_nat.c
+++ b/net/netfilter/nft_nat.c
@@ -108,6 +108,7 @@ static void nft_nat_eval(const struct nft_expr *expr,
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct = nf_ct_get(pkt->skb, &ctinfo);
 	struct nf_nat_range2 range;
+	int verdict;
 
 	memset(&range, 0, sizeof(range));
 
@@ -122,7 +123,12 @@ static void nft_nat_eval(const struct nft_expr *expr,
 
 	range.flags = priv->flags;
 
-	regs->verdict.code = nf_nat_setup_info(ct, &range, priv->type);
+	verdict = nf_nat_setup_info(ct, &range, priv->type);
+	if (verdict == NF_DROP)
+		verdict = NF_DROP_REASON(pkt->skb,
+					 SKB_DROP_REASON_NETFILTER_DROP,
+					 EPERM);
+	regs->verdict.code = verdict;
 }
 
 static const struct nla_policy nft_nat_policy[NFTA_NAT_MAX + 1] = {
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH nf-next 4/4] netfilter: nf_tables: use skb_drop_reason
  2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
                   ` (2 preceding siblings ...)
  2024-10-02 15:55 ` [PATCH nf-next 3/4] netfilter: nf_nat: use skb_drop_reason Florian Westphal
@ 2024-10-02 15:55 ` Florian Westphal
  2024-10-12 14:09 ` [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Pablo Neira Ayuso
  4 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-02 15:55 UTC (permalink / raw)
  To: netfilter-devel; +Cc: Florian Westphal

same as previous patch: extend nftables nat and masquerade functions to
indicate more precise drop locations.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter.h                 |  5 ++++-
 net/bridge/netfilter/nft_reject_bridge.c  |  2 +-
 net/ipv4/netfilter/nft_reject_ipv4.c      |  2 +-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c |  5 ++++-
 net/netfilter/nf_nat_proto.c              | 18 +++++++++---------
 net/netfilter/nf_nat_redirect.c           |  4 ++--
 net/netfilter/nf_synproxy_core.c          | 16 ++++++++--------
 net/netfilter/nft_chain_filter.c          |  4 ++--
 net/netfilter/nft_compat.c                |  8 ++++----
 net/netfilter/nft_connlimit.c             |  4 ++--
 net/netfilter/nft_ct.c                    | 14 ++++++++------
 net/netfilter/nft_exthdr.c                |  2 +-
 net/netfilter/nft_fib_inet.c              |  2 +-
 net/netfilter/nft_fwd_netdev.c            |  4 ++--
 net/netfilter/nft_reject_inet.c           |  2 +-
 net/netfilter/nft_reject_netdev.c         |  2 +-
 net/netfilter/nft_synproxy.c              | 10 +++++-----
 17 files changed, 56 insertions(+), 48 deletions(-)

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 2683b2b77612..5b350de9b4c7 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -25,7 +25,10 @@ static inline int NF_DROP_GETERR(int verdict)
 static __always_inline int
 NF_DROP_REASON(struct sk_buff *skb, enum skb_drop_reason reason, u32 err)
 {
-	BUILD_BUG_ON(err > 0xffff);
+	if (__builtin_constant_p(err))
+		BUILD_BUG_ON(err > 0xffff);
+	else if (WARN_ON_ONCE(err > 0xffff))
+		err = 0;
 
 	kfree_skb_reason(skb, reason);
 
diff --git a/net/bridge/netfilter/nft_reject_bridge.c b/net/bridge/netfilter/nft_reject_bridge.c
index 1cb5c16e97b7..09da1f4459c9 100644
--- a/net/bridge/netfilter/nft_reject_bridge.c
+++ b/net/bridge/netfilter/nft_reject_bridge.c
@@ -166,7 +166,7 @@ static void nft_reject_bridge_eval(const struct nft_expr *expr,
 		break;
 	}
 out:
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static int nft_reject_bridge_validate(const struct nft_ctx *ctx,
diff --git a/net/ipv4/netfilter/nft_reject_ipv4.c b/net/ipv4/netfilter/nft_reject_ipv4.c
index 6cb213bb7256..76d1f89b594a 100644
--- a/net/ipv4/netfilter/nft_reject_ipv4.c
+++ b/net/ipv4/netfilter/nft_reject_ipv4.c
@@ -34,7 +34,7 @@ static void nft_reject_ipv4_eval(const struct nft_expr *expr,
 		break;
 	}
 
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static struct nft_expr_type nft_reject_ipv4_type;
diff --git a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
index be7817fbc024..41f2a7906a5a 100644
--- a/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
+++ b/net/ipv6/netfilter/nf_defrag_ipv6_hooks.c
@@ -70,7 +70,10 @@ static unsigned int ipv6_defrag(void *priv,
 	if (err == -EINPROGRESS)
 		return NF_STOLEN;
 
-	return err == 0 ? NF_ACCEPT : NF_DROP;
+	if (err == 0)
+		return NF_ACCEPT;
+
+	return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 }
 
 static const struct nf_hook_ops ipv6_defrag_ops[] = {
diff --git a/net/netfilter/nf_nat_proto.c b/net/netfilter/nf_nat_proto.c
index dc450cc81222..7458c8b59ed2 100644
--- a/net/netfilter/nf_nat_proto.c
+++ b/net/netfilter/nf_nat_proto.c
@@ -439,7 +439,7 @@ unsigned int nf_nat_manip_pkt(struct sk_buff *skb, struct nf_conn *ct,
 		break;
 	}
 
-	return NF_DROP;
+	return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static void nf_nat_ipv4_csum_update(struct sk_buff *skb,
@@ -636,7 +636,7 @@ nf_nat_ipv4_fn(void *priv, struct sk_buff *skb,
 		if (ip_hdr(skb)->protocol == IPPROTO_ICMP) {
 			if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo,
 							   state->hook))
-				return NF_DROP;
+				return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			else
 				return NF_ACCEPT;
 		}
@@ -781,7 +781,7 @@ nf_nat_ipv4_out(void *priv, struct sk_buff *skb,
 		     ct->tuplehash[!dir].tuple.dst.u.all)) {
 			err = nf_xfrm_me_harder(state->net, skb, AF_INET);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 	}
 #endif
@@ -809,7 +809,7 @@ nf_nat_ipv4_local_fn(void *priv, struct sk_buff *skb,
 		    ct->tuplehash[!dir].tuple.src.u3.ip) {
 			err = ip_route_me_harder(state->net, state->sk, skb, RTN_UNSPEC);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 #ifdef CONFIG_XFRM
 		else if (!(IPCB(skb)->flags & IPSKB_XFRM_TRANSFORMED) &&
@@ -818,7 +818,7 @@ nf_nat_ipv4_local_fn(void *priv, struct sk_buff *skb,
 			 ct->tuplehash[!dir].tuple.src.u.all) {
 			err = nf_xfrm_me_harder(state->net, skb, AF_INET);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 #endif
 	}
@@ -965,7 +965,7 @@ nf_nat_ipv6_fn(void *priv, struct sk_buff *skb,
 			if (!nf_nat_icmpv6_reply_translation(skb, ct, ctinfo,
 							     state->hook,
 							     hdrlen))
-				return NF_DROP;
+				return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			else
 				return NF_ACCEPT;
 		}
@@ -1040,7 +1040,7 @@ nf_nat_ipv6_out(void *priv, struct sk_buff *skb,
 		     ct->tuplehash[!dir].tuple.dst.u.all)) {
 			err = nf_xfrm_me_harder(state->net, skb, AF_INET6);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 	}
 #endif
@@ -1069,7 +1069,7 @@ nf_nat_ipv6_local_fn(void *priv, struct sk_buff *skb,
 				      &ct->tuplehash[!dir].tuple.src.u3)) {
 			err = nf_ip6_route_me_harder(state->net, state->sk, skb);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 #ifdef CONFIG_XFRM
 		else if (!(IP6CB(skb)->flags & IP6SKB_XFRM_TRANSFORMED) &&
@@ -1078,7 +1078,7 @@ nf_nat_ipv6_local_fn(void *priv, struct sk_buff *skb,
 			 ct->tuplehash[!dir].tuple.src.u.all) {
 			err = nf_xfrm_me_harder(state->net, skb, AF_INET6);
 			if (err < 0)
-				ret = NF_DROP_ERR(err);
+				ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 		}
 #endif
 	}
diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index 5b37487d9d11..5adb648538f0 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -71,7 +71,7 @@ nf_nat_redirect_ipv4(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		}
 
 		if (!newdst.ip)
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 	}
 
 	return nf_nat_redirect(skb, range, &newdst);
@@ -130,7 +130,7 @@ nf_nat_redirect_ipv6(struct sk_buff *skb, const struct nf_nat_range2 *range,
 		}
 
 		if (!addr)
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 	}
 
 	return nf_nat_redirect(skb, range, &newdst);
diff --git a/net/netfilter/nf_synproxy_core.c b/net/netfilter/nf_synproxy_core.c
index 5b140c12b7df..08a650fc6ee7 100644
--- a/net/netfilter/nf_synproxy_core.c
+++ b/net/netfilter/nf_synproxy_core.c
@@ -664,7 +664,7 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 	thoff = ip_hdrlen(skb);
 	th = skb_header_pointer(skb, thoff, sizeof(_th), &_th);
 	if (!th)
-		return NF_DROP;
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 	state = &ct->proto.tcp;
 	switch (state->state) {
@@ -689,7 +689,7 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 		fallthrough;
 	case TCP_CONNTRACK_SYN_SENT:
 		if (!synproxy_parse_options(skb, thoff, th, &opts))
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 		if (!th->syn && th->ack &&
 		    CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
@@ -703,7 +703,7 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 				consume_skb(skb);
 				return NF_STOLEN;
 			} else {
-				return NF_DROP;
+				return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			}
 		}
 
@@ -718,7 +718,7 @@ ipv4_synproxy_hook(void *priv, struct sk_buff *skb,
 			break;
 
 		if (!synproxy_parse_options(skb, thoff, th, &opts))
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 		if (opts.options & NF_SYNPROXY_OPT_TIMESTAMP) {
 			synproxy->tsoff = opts.tsval - synproxy->its;
@@ -1087,7 +1087,7 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 
 	th = skb_header_pointer(skb, thoff, sizeof(_th), &_th);
 	if (!th)
-		return NF_DROP;
+		return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 	state = &ct->proto.tcp;
 	switch (state->state) {
@@ -1112,7 +1112,7 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 		fallthrough;
 	case TCP_CONNTRACK_SYN_SENT:
 		if (!synproxy_parse_options(skb, thoff, th, &opts))
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 		if (!th->syn && th->ack &&
 		    CTINFO2DIR(ctinfo) == IP_CT_DIR_ORIGINAL) {
@@ -1126,7 +1126,7 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 				consume_skb(skb);
 				return NF_STOLEN;
 			} else {
-				return NF_DROP;
+				return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			}
 		}
 
@@ -1141,7 +1141,7 @@ ipv6_synproxy_hook(void *priv, struct sk_buff *skb,
 			break;
 
 		if (!synproxy_parse_options(skb, thoff, th, &opts))
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 		if (opts.options & NF_SYNPROXY_OPT_TIMESTAMP) {
 			synproxy->tsoff = opts.tsval - synproxy->its;
diff --git a/net/netfilter/nft_chain_filter.c b/net/netfilter/nft_chain_filter.c
index 7010541fcca6..b0a90b49ada9 100644
--- a/net/netfilter/nft_chain_filter.c
+++ b/net/netfilter/nft_chain_filter.c
@@ -175,7 +175,7 @@ static unsigned int nft_do_chain_inet_ingress(void *priv, struct sk_buff *skb,
 		nft_set_pktinfo(&pkt, skb, &ingress_state);
 
 		if (nft_set_pktinfo_ipv4_ingress(&pkt) < 0)
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		break;
 	case htons(ETH_P_IPV6):
 		ingress_state.pf = NFPROTO_IPV6;
@@ -183,7 +183,7 @@ static unsigned int nft_do_chain_inet_ingress(void *priv, struct sk_buff *skb,
 		nft_set_pktinfo(&pkt, skb, &ingress_state);
 
 		if (nft_set_pktinfo_ipv6_ingress(&pkt) < 0)
-			return NF_DROP;
+			return NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		break;
 	default:
 		return NF_ACCEPT;
diff --git a/net/netfilter/nft_compat.c b/net/netfilter/nft_compat.c
index 52cdfee17f73..0ee10c7f7a4f 100644
--- a/net/netfilter/nft_compat.c
+++ b/net/netfilter/nft_compat.c
@@ -85,7 +85,7 @@ static void nft_target_eval_xt(const struct nft_expr *expr,
 	ret = target->target(skb, &xt);
 
 	if (xt.hotdrop)
-		ret = NF_DROP;
+		ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 	switch (ret) {
 	case XT_CONTINUE:
@@ -112,14 +112,14 @@ static void nft_target_eval_bridge(const struct nft_expr *expr,
 	ret = target->target(skb, &xt);
 
 	if (xt.hotdrop)
-		ret = NF_DROP;
+		ret = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 
 	switch (ret) {
 	case EBT_ACCEPT:
 		regs->verdict.code = NF_ACCEPT;
 		break;
 	case EBT_DROP:
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		break;
 	case EBT_CONTINUE:
 		regs->verdict.code = NFT_CONTINUE;
@@ -403,7 +403,7 @@ static void __nft_match_eval(const struct nft_expr *expr,
 	ret = match->match(skb, &xt);
 
 	if (xt.hotdrop) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
diff --git a/net/netfilter/nft_connlimit.c b/net/netfilter/nft_connlimit.c
index 92b984fa8175..3cfa9295001a 100644
--- a/net/netfilter/nft_connlimit.c
+++ b/net/netfilter/nft_connlimit.c
@@ -39,12 +39,12 @@ static inline void nft_connlimit_do_eval(struct nft_connlimit *priv,
 		zone = nf_ct_zone(ct);
 	} else if (!nf_ct_get_tuplepr(pkt->skb, skb_network_offset(pkt->skb),
 				      nft_pf(pkt), nft_net(pkt), &tuple)) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
 	if (nf_conncount_add(nft_net(pkt), priv->list, tuple_ptr, zone)) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
diff --git a/net/netfilter/nft_ct.c b/net/netfilter/nft_ct.c
index 67a41cd2baaf..d705077ccaaa 100644
--- a/net/netfilter/nft_ct.c
+++ b/net/netfilter/nft_ct.c
@@ -259,7 +259,7 @@ static void nft_ct_set_zone_eval(const struct nft_expr *expr,
 		 */
 		ct = nf_ct_tmpl_alloc(nft_net(pkt), &zone, GFP_ATOMIC);
 		if (!ct) {
-			regs->verdict.code = NF_DROP;
+			regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NOMEM, ENOMEM);
 			return;
 		}
 		__set_bit(IPS_CONFIRMED_BIT, &ct->status);
@@ -917,7 +917,7 @@ static void nft_ct_timeout_obj_eval(struct nft_object *obj,
 	if (!timeout) {
 		timeout = nf_ct_timeout_ext_add(ct, priv->timeout, GFP_ATOMIC);
 		if (!timeout) {
-			regs->verdict.code = NF_DROP;
+			regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NOMEM, ENOMEM);
 			return;
 		}
 	}
@@ -1316,6 +1316,7 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 	enum ip_conntrack_dir dir;
 	u16 l3num = priv->l3num;
 	struct nf_conn *ct;
+	int err;
 
 	ct = nf_ct_get(pkt->skb, &ctinfo);
 	if (!ct || nf_ct_is_confirmed(ct) || nf_ct_is_template(ct)) {
@@ -1328,7 +1329,7 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 	if (!help)
 		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
 	if (!help) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NOMEM, ENOMEM);
 		return;
 	}
 
@@ -1341,7 +1342,7 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 
 	exp = nf_ct_expect_alloc(ct);
 	if (exp == NULL) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NOMEM, ENOMEM);
 		return;
 	}
 	nf_ct_expect_init(exp, NF_CT_EXPECT_CLASS_DEFAULT, l3num,
@@ -1350,8 +1351,9 @@ static void nft_ct_expect_obj_eval(struct nft_object *obj,
 		          priv->l4proto, NULL, &priv->dport);
 	exp->timeout.expires = jiffies + priv->timeout * HZ;
 
-	if (nf_ct_expect_related(exp, 0) != 0)
-		regs->verdict.code = NF_DROP;
+	err = nf_ct_expect_related(exp, 0);
+	if (err != 0)
+		regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, -err);
 }
 
 static const struct nla_policy nft_ct_expect_policy[NFTA_CT_EXPECT_MAX + 1] = {
diff --git a/net/netfilter/nft_exthdr.c b/net/netfilter/nft_exthdr.c
index 6bfd33516241..9e7fedcb2c16 100644
--- a/net/netfilter/nft_exthdr.c
+++ b/net/netfilter/nft_exthdr.c
@@ -365,7 +365,7 @@ static void nft_exthdr_tcp_strip_eval(const struct nft_expr *expr,
 	return;
 drop:
 	/* can't remove, no choice but to drop */
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static void nft_exthdr_sctp_eval(const struct nft_expr *expr,
diff --git a/net/netfilter/nft_fib_inet.c b/net/netfilter/nft_fib_inet.c
index 666a3741d20b..a15f40729201 100644
--- a/net/netfilter/nft_fib_inet.c
+++ b/net/netfilter/nft_fib_inet.c
@@ -38,7 +38,7 @@ static void nft_fib_inet_eval(const struct nft_expr *expr,
 		break;
 	}
 
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static struct nft_expr_type nft_fib_inet_type;
diff --git a/net/netfilter/nft_fwd_netdev.c b/net/netfilter/nft_fwd_netdev.c
index 152a9fb4d23a..ecf48b5e8de1 100644
--- a/net/netfilter/nft_fwd_netdev.c
+++ b/net/netfilter/nft_fwd_netdev.c
@@ -112,7 +112,7 @@ static void nft_fwd_neigh_eval(const struct nft_expr *expr,
 			goto out;
 		}
 		if (skb_try_make_writable(skb, sizeof(*iph))) {
-			verdict = NF_DROP;
+			verdict = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			goto out;
 		}
 		iph = ip_hdr(skb);
@@ -128,7 +128,7 @@ static void nft_fwd_neigh_eval(const struct nft_expr *expr,
 			goto out;
 		}
 		if (skb_try_make_writable(skb, sizeof(*ip6h))) {
-			verdict = NF_DROP;
+			verdict = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 			goto out;
 		}
 		ip6h = ipv6_hdr(skb);
diff --git a/net/netfilter/nft_reject_inet.c b/net/netfilter/nft_reject_inet.c
index 49020e67304a..6ba969e94afa 100644
--- a/net/netfilter/nft_reject_inet.c
+++ b/net/netfilter/nft_reject_inet.c
@@ -57,7 +57,7 @@ static void nft_reject_inet_eval(const struct nft_expr *expr,
 		break;
 	}
 
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static int nft_reject_inet_validate(const struct nft_ctx *ctx,
diff --git a/net/netfilter/nft_reject_netdev.c b/net/netfilter/nft_reject_netdev.c
index 2558ce1505d9..ba39bf9274b1 100644
--- a/net/netfilter/nft_reject_netdev.c
+++ b/net/netfilter/nft_reject_netdev.c
@@ -141,7 +141,7 @@ static void nft_reject_netdev_eval(const struct nft_expr *expr,
 		break;
 	}
 out:
-	regs->verdict.code = NF_DROP;
+	regs->verdict.code = NF_DROP_REASON(pkt->skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 }
 
 static int nft_reject_netdev_validate(const struct nft_ctx *ctx,
diff --git a/net/netfilter/nft_synproxy.c b/net/netfilter/nft_synproxy.c
index 5d3e51825985..fe96f6fc4d3c 100644
--- a/net/netfilter/nft_synproxy.c
+++ b/net/netfilter/nft_synproxy.c
@@ -66,7 +66,7 @@ static void nft_synproxy_eval_v4(const struct nft_synproxy *priv,
 			consume_skb(skb);
 			regs->verdict.code = NF_STOLEN;
 		} else {
-			regs->verdict.code = NF_DROP;
+			regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		}
 	}
 }
@@ -97,7 +97,7 @@ static void nft_synproxy_eval_v6(const struct nft_synproxy *priv,
 			consume_skb(skb);
 			regs->verdict.code = NF_STOLEN;
 		} else {
-			regs->verdict.code = NF_DROP;
+			regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		}
 	}
 }
@@ -119,7 +119,7 @@ static void nft_synproxy_do_eval(const struct nft_synproxy *priv,
 	}
 
 	if (nf_ip_checksum(skb, nft_hook(pkt), thoff, IPPROTO_TCP)) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
@@ -127,12 +127,12 @@ static void nft_synproxy_do_eval(const struct nft_synproxy *priv,
 				 sizeof(struct tcphdr),
 				 &_tcph);
 	if (!tcp) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
 	if (!synproxy_parse_options(skb, thoff, tcp, &opts)) {
-		regs->verdict.code = NF_DROP;
+		regs->verdict.code = NF_DROP_REASON(skb, SKB_DROP_REASON_NETFILTER_DROP, EPERM);
 		return;
 	}
 
-- 
2.45.2


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
                   ` (3 preceding siblings ...)
  2024-10-02 15:55 ` [PATCH nf-next 4/4] netfilter: nf_tables: " Florian Westphal
@ 2024-10-12 14:09 ` Pablo Neira Ayuso
  2024-10-12 14:42   ` Florian Westphal
  4 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2024-10-12 14:09 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

Hi Florian,

On Wed, Oct 02, 2024 at 05:55:38PM +0200, Florian Westphal wrote:
> Provide more precise drop information rather than doing the freeing
> from core.c:nf_hook_slow().
> 
> First patch is a small preparation patch, rest coverts NF_DROP
> locations of NF_DROP_REASON().

One question regarding this series.

Most spots still rely on EPERM which is the default reason for
NF_DROP.

I wonder if it is worth updating all these spots to use NF_DROP_REASON
with EPERM. I think patchset becomes smaller if it is only used to
provide a better reason than EPERM.

Thanks.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-12 14:09 ` [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Pablo Neira Ayuso
@ 2024-10-12 14:42   ` Florian Westphal
  2024-10-12 15:42     ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Westphal @ 2024-10-12 14:42 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> One question regarding this series.
> 
> Most spots still rely on EPERM which is the default reason for
> NF_DROP.

core converts NF_DROP to EPERM if no errno value is set, correct.

> I wonder if it is worth updating all these spots to use NF_DROP_REASON
> with EPERM. I think patchset becomes smaller if it is only used to
> provide a better reason than EPERM.

I'm not following, sorry.  What do you mean?

This is not about errno.  NF_DROP_REASON() calls kfree_skb, so tooling
can show location other than nf_hook_slow().

Or do you mean using a different macro that always sets EPERM?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-12 14:42   ` Florian Westphal
@ 2024-10-12 15:42     ` Pablo Neira Ayuso
  2024-10-12 15:54       ` Florian Westphal
  0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2024-10-12 15:42 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Sat, Oct 12, 2024 at 04:42:16PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > One question regarding this series.
> > 
> > Most spots still rely on EPERM which is the default reason for
> > NF_DROP.
> 
> core converts NF_DROP to EPERM if no errno value is set, correct.
> 
> > I wonder if it is worth updating all these spots to use NF_DROP_REASON
> > with EPERM. I think patchset becomes smaller if it is only used to
> > provide a better reason than EPERM.
> 
> I'm not following, sorry.  What do you mean?
> 
> This is not about errno.  NF_DROP_REASON() calls kfree_skb, so tooling
> can show location other than nf_hook_slow().

Right.

> Or do you mean using a different macro that always sets EPERM?

Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is
shorter?

        NF_DROP_REASON(pkt->skb, -EPERM)

And add a new macro for br_netfilter NF_BR_DROP_REASON which does not
always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for
this new macro if you like).

Or you think the existing generic long version of NF_DROP_REASON is
convenient to have?

Thanks

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-12 15:42     ` Pablo Neira Ayuso
@ 2024-10-12 15:54       ` Florian Westphal
  2024-10-12 16:45         ` Pablo Neira Ayuso
  0 siblings, 1 reply; 11+ messages in thread
From: Florian Westphal @ 2024-10-12 15:54 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Or do you mean using a different macro that always sets EPERM?
> 
> Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is
> shorter?
> 
>         NF_DROP_REASON(pkt->skb, -EPERM)
> 
> And add a new macro for br_netfilter NF_BR_DROP_REASON which does not
> always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for
> this new macro if you like).

NF_DROP_REASON is already in the tree and currently most users use
something other than SKB_DROP_REASON_NETFILTER_DROP.

I did not yet add new enum values or a dedicated nf namespace
(enum skb_drop_reason_subsys), because I did not see a reason and
wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat,
whatever).

If you like, I can add NF_FREE_SKB(skb, errno) and rework this
set to use that?


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-12 15:54       ` Florian Westphal
@ 2024-10-12 16:45         ` Pablo Neira Ayuso
  2024-10-12 20:38           ` Florian Westphal
  0 siblings, 1 reply; 11+ messages in thread
From: Pablo Neira Ayuso @ 2024-10-12 16:45 UTC (permalink / raw)
  To: Florian Westphal; +Cc: netfilter-devel

On Sat, Oct 12, 2024 at 05:54:48PM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > Or do you mean using a different macro that always sets EPERM?
> > 
> > Maybe remove SKB_DROP_REASON_NETFILTER_DROP from macro, so line is
> > shorter?
> > 
> >         NF_DROP_REASON(pkt->skb, -EPERM)
> > 
> > And add a new macro for br_netfilter NF_BR_DROP_REASON which does not
> > always sets SKB_DROP_REASON_NETFILTER_DROP? (Pick a better name for
> > this new macro if you like).
> 
> NF_DROP_REASON is already in the tree and currently most users use
> something other than SKB_DROP_REASON_NETFILTER_DROP.
> 
> I did not yet add new enum values or a dedicated nf namespace
> (enum skb_drop_reason_subsys), because I did not see a reason and
> wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat,
> whatever).

Does this mean values exposed through tracing infrastructure can
change or these are part of uapi? From what I read from you, I
understand it is possible to change SKB_DROP_REASON_NETFILTER_DROP to
a more specific sub-subsystem tag in the future without issues.

> If you like, I can add NF_FREE_SKB(skb, errno) and rework this
> set to use that?

Not strong about this. I was exploring if it should be possible to
remove (repetitive) information in the code that can be assumed to be
implicit, I still like the word "REASON" in the macro for grepping.

I think we can just move on with this series as-is if you prefer and
add new macros incrementally to refine.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places
  2024-10-12 16:45         ` Pablo Neira Ayuso
@ 2024-10-12 20:38           ` Florian Westphal
  0 siblings, 0 replies; 11+ messages in thread
From: Florian Westphal @ 2024-10-12 20:38 UTC (permalink / raw)
  To: Pablo Neira Ayuso; +Cc: Florian Westphal, netfilter-devel

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > I did not yet add new enum values or a dedicated nf namespace
> > (enum skb_drop_reason_subsys), because I did not see a reason and
> > wasn't sure if we'd need sub-subsystems (nf_tables, conntrack, nat,
> > whatever).
> 
> Does this mean values exposed through tracing infrastructure can
> change or these are part of uapi?

The enum has had values added (not just appended), so its not considered
uapi.

> From what I read from you, I
> understand it is possible to change SKB_DROP_REASON_NETFILTER_DROP to
> a more specific sub-subsystem tag in the future without issues.

Thats correct.

> > If you like, I can add NF_FREE_SKB(skb, errno) and rework this
> > set to use that?
> 
> Not strong about this. I was exploring if it should be possible to
> remove (repetitive) information in the code that can be assumed to be
> implicit, I still like the word "REASON" in the macro for grepping.

Yes, common name is good.

> I think we can just move on with this series as-is if you prefer and
> add new macros incrementally to refine.

I think we can refine later if there are use-cases for that.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-10-12 20:38 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-10-02 15:55 [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Florian Westphal
2024-10-02 15:55 ` [PATCH nf-next 1/4] netfilter: xt_nat: compact nf_nat_setup_info calls Florian Westphal
2024-10-02 15:55 ` [PATCH nf-next 2/4] netfilter: xt_nat: drop packet earlier Florian Westphal
2024-10-02 15:55 ` [PATCH nf-next 3/4] netfilter: nf_nat: use skb_drop_reason Florian Westphal
2024-10-02 15:55 ` [PATCH nf-next 4/4] netfilter: nf_tables: " Florian Westphal
2024-10-12 14:09 ` [PATCH nf-next 0/4] netfilter: use skb_drop_reason in more places Pablo Neira Ayuso
2024-10-12 14:42   ` Florian Westphal
2024-10-12 15:42     ` Pablo Neira Ayuso
2024-10-12 15:54       ` Florian Westphal
2024-10-12 16:45         ` Pablo Neira Ayuso
2024-10-12 20:38           ` Florian Westphal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).