* [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: @ 2022-07-07 19:17 Justin Stitt 2022-07-08 23:33 ` Nick Desaulniers 2022-07-11 9:04 ` Pablo Neira Ayuso 0 siblings, 2 replies; 11+ messages in thread From: Justin Stitt @ 2022-07-07 19:17 UTC (permalink / raw) To: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni Cc: Nathan Chancellor, Nick Desaulniers, Tom Rix, netfilter-devel, coreteam, netdev, linux-kernel, llvm, Justin Stitt When building with Clang we encounter these warnings: | net/netfilter/xt_TPROXY.c:173:5: error: format specifies type 'unsigned | char' but the argument has type 'int' [-Werror,-Wformat] tproto, | &iph->saddr, ntohs(hp->source), - | net/netfilter/xt_TPROXY.c:181:4: error: format specifies type 'unsigned | char' but the argument has type 'int' [-Werror,-Wformat] tproto, | &iph->saddr, ntohs(hp->source), The format specifier `%hhu` refers to a u8 while tproto is an int. In this case we weren't losing any data because ipv6_find_hdr returns an int but its return value (nexthdr) is a u8. This u8 gets widened to an int when returned from ipv6_find_hdr and assigned to tproto. The previous format specifier is functionally fine but still produces a warning due to a type mismatch. The fix is simply to listen to Clang and change `%hhu` to `%d` for both instances of the warning. Link: https://github.com/ClangBuiltLinux/linux/issues/378 Signed-off-by: Justin Stitt <justinstitt@google.com> --- It should be noted that for this entire file to build without -Wformat warnings you should apply this `ntohs` patch which fixed many, many -Wformat warnings in the kernel. https://lore.kernel.org/all/20220608223539.470472-1-justinstitt@google.com/ net/netfilter/xt_TPROXY.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c index 459d0696c91a..5d74abffc94f 100644 --- a/net/netfilter/xt_TPROXY.c +++ b/net/netfilter/xt_TPROXY.c @@ -169,7 +169,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) targets on the same rule yet */ skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", + pr_debug("redirecting: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", tproto, &iph->saddr, ntohs(hp->source), laddr, ntohs(lport), skb->mark); @@ -177,7 +177,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) return NF_ACCEPT; } - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", + pr_debug("no socket, dropping: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", tproto, &iph->saddr, ntohs(hp->source), &iph->daddr, ntohs(hp->dest), skb->mark); -- 2.37.0.rc0.161.g10f37bed90-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: 2022-07-07 19:17 [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: Justin Stitt @ 2022-07-08 23:33 ` Nick Desaulniers 2022-07-11 9:04 ` Pablo Neira Ayuso 1 sibling, 0 replies; 11+ messages in thread From: Nick Desaulniers @ 2022-07-08 23:33 UTC (permalink / raw) To: Justin Stitt Cc: Pablo Neira Ayuso, Jozsef Kadlecsik, Florian Westphal, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Tom Rix, netfilter-devel, coreteam, netdev, linux-kernel, llvm On Thu, Jul 7, 2022 at 12:18 PM Justin Stitt <justinstitt@google.com> wrote: > > When building with Clang we encounter these warnings: > | net/netfilter/xt_TPROXY.c:173:5: error: format specifies type 'unsigned > | char' but the argument has type 'int' [-Werror,-Wformat] tproto, > | &iph->saddr, ntohs(hp->source), > - > | net/netfilter/xt_TPROXY.c:181:4: error: format specifies type 'unsigned > | char' but the argument has type 'int' [-Werror,-Wformat] tproto, > | &iph->saddr, ntohs(hp->source), > > The format specifier `%hhu` refers to a u8 while tproto is an int. In > this case we weren't losing any data because ipv6_find_hdr returns an > int but its return value (nexthdr) is a u8. This u8 gets widened to an > int when returned from ipv6_find_hdr and assigned to tproto. The > previous format specifier is functionally fine but still produces a > warning due to a type mismatch. > > The fix is simply to listen to Clang and change `%hhu` to `%d` for both > instances of the warning. > > Link: https://github.com/ClangBuiltLinux/linux/issues/378 > Signed-off-by: Justin Stitt <justinstitt@google.com> Thanks for the patch, this fixes the warning I observe when building ARCH=arm64 allmodconfig with -Wno-format removed! Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> Tested-by: Nick Desaulniers <ndesaulniers@google.com> > --- > It should be noted that for this entire file to build without -Wformat > warnings you should apply this `ntohs` patch which fixed many, many > -Wformat warnings in the kernel. > https://lore.kernel.org/all/20220608223539.470472-1-justinstitt@google.com/ > > net/netfilter/xt_TPROXY.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > index 459d0696c91a..5d74abffc94f 100644 > --- a/net/netfilter/xt_TPROXY.c > +++ b/net/netfilter/xt_TPROXY.c > @@ -169,7 +169,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > targets on the same rule yet */ > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > + pr_debug("redirecting: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > tproto, &iph->saddr, ntohs(hp->source), > laddr, ntohs(lport), skb->mark); > > @@ -177,7 +177,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > return NF_ACCEPT; > } > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > + pr_debug("no socket, dropping: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > tproto, &iph->saddr, ntohs(hp->source), > &iph->daddr, ntohs(hp->dest), skb->mark); > > -- > 2.37.0.rc0.161.g10f37bed90-goog > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: 2022-07-07 19:17 [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: Justin Stitt 2022-07-08 23:33 ` Nick Desaulniers @ 2022-07-11 9:04 ` Pablo Neira Ayuso 2022-07-11 19:44 ` Justin Stitt 1 sibling, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2022-07-11 9:04 UTC (permalink / raw) To: Justin Stitt Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Nick Desaulniers, Tom Rix, netfilter-devel, coreteam, netdev, linux-kernel, llvm On Thu, Jul 07, 2022 at 12:17:45PM -0700, Justin Stitt wrote: > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > index 459d0696c91a..5d74abffc94f 100644 > --- a/net/netfilter/xt_TPROXY.c > +++ b/net/netfilter/xt_TPROXY.c > @@ -169,7 +169,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > targets on the same rule yet */ > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > + pr_debug("redirecting: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > tproto, &iph->saddr, ntohs(hp->source), > laddr, ntohs(lport), skb->mark); > > @@ -177,7 +177,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > return NF_ACCEPT; > } > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > + pr_debug("no socket, dropping: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > tproto, &iph->saddr, ntohs(hp->source), > &iph->daddr, ntohs(hp->dest), skb->mark); Could you instead send a patch to remove these pr_debug calls? Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: 2022-07-11 9:04 ` Pablo Neira Ayuso @ 2022-07-11 19:44 ` Justin Stitt 2022-07-12 8:40 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Justin Stitt @ 2022-07-11 19:44 UTC (permalink / raw) To: Pablo Neira Ayuso Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Nick Desaulniers, Tom Rix, netfilter-devel, coreteam, netdev, linux-kernel, llvm On Mon, Jul 11, 2022 at 2:04 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Thu, Jul 07, 2022 at 12:17:45PM -0700, Justin Stitt wrote: > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > > index 459d0696c91a..5d74abffc94f 100644 > > --- a/net/netfilter/xt_TPROXY.c > > +++ b/net/netfilter/xt_TPROXY.c > > @@ -169,7 +169,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > targets on the same rule yet */ > > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > + pr_debug("redirecting: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > > tproto, &iph->saddr, ntohs(hp->source), > > laddr, ntohs(lport), skb->mark); > > > > @@ -177,7 +177,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > return NF_ACCEPT; > > } > > > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > + pr_debug("no socket, dropping: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > > tproto, &iph->saddr, ntohs(hp->source), > > &iph->daddr, ntohs(hp->dest), skb->mark); > > Could you instead send a patch to remove these pr_debug calls? Do you mean all Instances of pr_debug in `xt_TPROXY.c` (of which there are six) or just these two specific cases @ +169 and +177. > Thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: 2022-07-11 19:44 ` Justin Stitt @ 2022-07-12 8:40 ` Pablo Neira Ayuso 2022-07-12 18:34 ` [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations Justin Stitt 0 siblings, 1 reply; 11+ messages in thread From: Pablo Neira Ayuso @ 2022-07-12 8:40 UTC (permalink / raw) To: Justin Stitt Cc: Jozsef Kadlecsik, Florian Westphal, David S . Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Nathan Chancellor, Nick Desaulniers, Tom Rix, netfilter-devel, coreteam, netdev, linux-kernel, llvm On Mon, Jul 11, 2022 at 12:44:05PM -0700, Justin Stitt wrote: > On Mon, Jul 11, 2022 at 2:04 AM Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > On Thu, Jul 07, 2022 at 12:17:45PM -0700, Justin Stitt wrote: > > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > > > index 459d0696c91a..5d74abffc94f 100644 > > > --- a/net/netfilter/xt_TPROXY.c > > > +++ b/net/netfilter/xt_TPROXY.c > > > @@ -169,7 +169,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > > targets on the same rule yet */ > > > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > > > > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > > + pr_debug("redirecting: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > > > tproto, &iph->saddr, ntohs(hp->source), > > > laddr, ntohs(lport), skb->mark); > > > > > > @@ -177,7 +177,7 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > > return NF_ACCEPT; > > > } > > > > > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > > + pr_debug("no socket, dropping: proto %d %pI6:%hu -> %pI6:%hu, mark: %x\n", > > > tproto, &iph->saddr, ntohs(hp->source), > > > &iph->daddr, ntohs(hp->dest), skb->mark); > > > > Could you instead send a patch to remove these pr_debug calls? > > Do you mean all Instances of pr_debug in `xt_TPROXY.c` (of which there > are six) or just these two specific cases @ +169 and +177. Yes, remove all pr_debug instances, thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-12 8:40 ` Pablo Neira Ayuso @ 2022-07-12 18:34 ` Justin Stitt 2022-07-12 18:56 ` Nathan Chancellor 0 siblings, 1 reply; 11+ messages in thread From: Justin Stitt @ 2022-07-12 18:34 UTC (permalink / raw) To: pablo Cc: coreteam, davem, edumazet, fw, justinstitt, kadlec, kuba, linux-kernel, llvm, nathan, ndesaulniers, netdev, netfilter-devel, pabeni, trix pr_debug calls are no longer needed in this file. Pablo suggested "a patch to remove these pr_debug calls". This patch has some other beneficial collateral as it also silences multiple Clang -Wformat warnings that were present in the pr_debug calls. Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Justin Stitt <justinstitt@google.com> --- Suggestion here: https://lore.kernel.org/all/Ys0zZACWwGilTwHx@salvia/ net/netfilter/xt_TPROXY.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c index 459d0696c91a..dc7284e6357b 100644 --- a/net/netfilter/xt_TPROXY.c +++ b/net/netfilter/xt_TPROXY.c @@ -74,18 +74,10 @@ tproxy_tg4(struct net *net, struct sk_buff *skb, __be32 laddr, __be16 lport, /* This should be in a separate target, but we don't do multiple targets on the same rule yet */ skb->mark = (skb->mark & ~mark_mask) ^ mark_value; - - pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", - iph->protocol, &iph->daddr, ntohs(hp->dest), - &laddr, ntohs(lport), skb->mark); - nf_tproxy_assign_sock(skb, sk); return NF_ACCEPT; } - pr_debug("no socket, dropping: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", - iph->protocol, &iph->saddr, ntohs(hp->source), - &iph->daddr, ntohs(hp->dest), skb->mark); return NF_DROP; } @@ -123,13 +115,11 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL); if (tproto < 0) { - pr_debug("unable to find transport header in IPv6 packet, dropping\n"); return NF_DROP; } hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr); if (hp == NULL) { - pr_debug("unable to grab transport header contents in IPv6 packet, dropping\n"); return NF_DROP; } @@ -168,19 +158,10 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) /* This should be in a separate target, but we don't do multiple targets on the same rule yet */ skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; - - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", - tproto, &iph->saddr, ntohs(hp->source), - laddr, ntohs(lport), skb->mark); - nf_tproxy_assign_sock(skb, sk); return NF_ACCEPT; } - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", - tproto, &iph->saddr, ntohs(hp->source), - &iph->daddr, ntohs(hp->dest), skb->mark); - return NF_DROP; } -- 2.37.0.144.g8ac04bfd2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-12 18:34 ` [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations Justin Stitt @ 2022-07-12 18:56 ` Nathan Chancellor 2022-07-12 20:38 ` Justin Stitt 2022-07-12 20:49 ` [PATCH v2] " Justin Stitt 0 siblings, 2 replies; 11+ messages in thread From: Nathan Chancellor @ 2022-07-12 18:56 UTC (permalink / raw) To: Justin Stitt Cc: pablo, coreteam, davem, edumazet, fw, kadlec, kuba, linux-kernel, llvm, ndesaulniers, netdev, netfilter-devel, pabeni, trix Hi Justin, On Tue, Jul 12, 2022 at 11:34:52AM -0700, Justin Stitt wrote: > pr_debug calls are no longer needed in this file. > > Pablo suggested "a patch to remove these pr_debug calls". This patch has > some other beneficial collateral as it also silences multiple Clang > -Wformat warnings that were present in the pr_debug calls. > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org> > Signed-off-by: Justin Stitt <justinstitt@google.com> Thanks for the patch! Reviewed-by: Nathan Chancellor <nathan@kernel.org> Couple of style comments below that probably warrant a v2, you can carry the above tag forward for future revisions. No need to give me a "Suggested-by". > --- > Suggestion here: https://lore.kernel.org/all/Ys0zZACWwGilTwHx@salvia/ > > net/netfilter/xt_TPROXY.c | 19 ------------------- > 1 file changed, 19 deletions(-) > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > index 459d0696c91a..dc7284e6357b 100644 > --- a/net/netfilter/xt_TPROXY.c > +++ b/net/netfilter/xt_TPROXY.c > @@ -74,18 +74,10 @@ tproxy_tg4(struct net *net, struct sk_buff *skb, __be32 laddr, __be16 lport, > /* This should be in a separate target, but we don't do multiple > targets on the same rule yet */ > skb->mark = (skb->mark & ~mark_mask) ^ mark_value; > - > - pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > - iph->protocol, &iph->daddr, ntohs(hp->dest), > - &laddr, ntohs(lport), skb->mark); > - > nf_tproxy_assign_sock(skb, sk); > return NF_ACCEPT; > } > > - pr_debug("no socket, dropping: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > - iph->protocol, &iph->saddr, ntohs(hp->source), > - &iph->daddr, ntohs(hp->dest), skb->mark); > return NF_DROP; > } > > @@ -123,13 +115,11 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL); > if (tproto < 0) { checkpatch.pl should have warned that these if statement braces here and below are no longer necessary because there is only one statement within them now. if (tproto < 0) return NF_DROP; I believe it is important to do these types of style cleanups when doing a larger change so that people do not try to do them as standalone changes, which can irritate maintainers. > - pr_debug("unable to find transport header in IPv6 packet, dropping\n"); > return NF_DROP; > } > > hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr); > if (hp == NULL) { > - pr_debug("unable to grab transport header contents in IPv6 packet, dropping\n"); > return NF_DROP; > } if (hp == NULL) return NF_DROP; could even go a step farther and make it if (!hp) return NF_DROP; if there is a warning about that. > > @@ -168,19 +158,10 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > /* This should be in a separate target, but we don't do multiple > targets on the same rule yet */ > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > - > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > - tproto, &iph->saddr, ntohs(hp->source), > - laddr, ntohs(lport), skb->mark); > - > nf_tproxy_assign_sock(skb, sk); > return NF_ACCEPT; > } > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > - tproto, &iph->saddr, ntohs(hp->source), > - &iph->daddr, ntohs(hp->dest), skb->mark); > - > return NF_DROP; > } > > -- > 2.37.0.144.g8ac04bfd2-goog > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-12 18:56 ` Nathan Chancellor @ 2022-07-12 20:38 ` Justin Stitt 2022-07-12 20:49 ` [PATCH v2] " Justin Stitt 1 sibling, 0 replies; 11+ messages in thread From: Justin Stitt @ 2022-07-12 20:38 UTC (permalink / raw) To: Nathan Chancellor Cc: pablo, coreteam, davem, edumazet, fw, kadlec, kuba, linux-kernel, llvm, ndesaulniers, netdev, netfilter-devel, pabeni, trix On Tue, Jul 12, 2022 at 11:56 AM Nathan Chancellor <nathan@kernel.org> wrote: > > Hi Justin, > > On Tue, Jul 12, 2022 at 11:34:52AM -0700, Justin Stitt wrote: > > pr_debug calls are no longer needed in this file. > > > > Pablo suggested "a patch to remove these pr_debug calls". This patch has > > some other beneficial collateral as it also silences multiple Clang > > -Wformat warnings that were present in the pr_debug calls. > > > > Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org> > > Signed-off-by: Justin Stitt <justinstitt@google.com> > > Thanks for the patch! > > Reviewed-by: Nathan Chancellor <nathan@kernel.org> > > Couple of style comments below that probably warrant a v2, you can carry > the above tag forward for future revisions. No need to give me a > "Suggested-by". > > > --- > > Suggestion here: https://lore.kernel.org/all/Ys0zZACWwGilTwHx@salvia/ > > > > net/netfilter/xt_TPROXY.c | 19 ------------------- > > 1 file changed, 19 deletions(-) > > > > diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c > > index 459d0696c91a..dc7284e6357b 100644 > > --- a/net/netfilter/xt_TPROXY.c > > +++ b/net/netfilter/xt_TPROXY.c > > @@ -74,18 +74,10 @@ tproxy_tg4(struct net *net, struct sk_buff *skb, __be32 laddr, __be16 lport, > > /* This should be in a separate target, but we don't do multiple > > targets on the same rule yet */ > > skb->mark = (skb->mark & ~mark_mask) ^ mark_value; > > - > > - pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > > - iph->protocol, &iph->daddr, ntohs(hp->dest), > > - &laddr, ntohs(lport), skb->mark); > > - > > nf_tproxy_assign_sock(skb, sk); > > return NF_ACCEPT; > > } > > > > - pr_debug("no socket, dropping: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", > > - iph->protocol, &iph->saddr, ntohs(hp->source), > > - &iph->daddr, ntohs(hp->dest), skb->mark); > > return NF_DROP; > > } > > > > @@ -123,13 +115,11 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > > > tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL); > > if (tproto < 0) { > > checkpatch.pl should have warned that these if statement braces here and > below are no longer necessary because there is only one statement within > them now. Weirdly, checkpatch.pl gave 0 warnings regarding this patch. At any rate, v2 is coming shortly. Thanks for the review! > > if (tproto < 0) > return NF_DROP; > > I believe it is important to do these types of style cleanups when doing > a larger change so that people do not try to do them as standalone > changes, which can irritate maintainers. > > > - pr_debug("unable to find transport header in IPv6 packet, dropping\n"); > > return NF_DROP; > > } > > > > hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr); > > if (hp == NULL) { > > - pr_debug("unable to grab transport header contents in IPv6 packet, dropping\n"); > > return NF_DROP; > > } > > if (hp == NULL) > return NF_DROP; > > could even go a step farther and make it > > if (!hp) > return NF_DROP; > > if there is a warning about that. > > > > > @@ -168,19 +158,10 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) > > /* This should be in a separate target, but we don't do multiple > > targets on the same rule yet */ > > skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; > > - > > - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > - tproto, &iph->saddr, ntohs(hp->source), > > - laddr, ntohs(lport), skb->mark); > > - > > nf_tproxy_assign_sock(skb, sk); > > return NF_ACCEPT; > > } > > > > - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", > > - tproto, &iph->saddr, ntohs(hp->source), > > - &iph->daddr, ntohs(hp->dest), skb->mark); > > - > > return NF_DROP; > > } > > > > -- > > 2.37.0.144.g8ac04bfd2-goog > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-12 18:56 ` Nathan Chancellor 2022-07-12 20:38 ` Justin Stitt @ 2022-07-12 20:49 ` Justin Stitt 2022-07-18 17:43 ` Justin Stitt 1 sibling, 1 reply; 11+ messages in thread From: Justin Stitt @ 2022-07-12 20:49 UTC (permalink / raw) To: nathan Cc: coreteam, davem, edumazet, fw, justinstitt, kadlec, kuba, linux-kernel, llvm, ndesaulniers, netdev, netfilter-devel, pabeni, pablo, trix pr_debug calls are no longer needed in this file. Pablo suggested "a patch to remove these pr_debug calls". This patch has some other beneficial collateral as it also silences multiple Clang -Wformat warnings that were present in the pr_debug calls. Suggested-by: Pablo Neira Ayuso <pablo@netfilter.org> Reviewed-by: Nathan Chancellor <nathan@kernel.org> Signed-off-by: Justin Stitt <justinstitt@google.com> --- diff from v1 -> v2: * converted if statement one-liner style * x == NULL is now !x net/netfilter/xt_TPROXY.c | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/net/netfilter/xt_TPROXY.c b/net/netfilter/xt_TPROXY.c index 459d0696c91a..e4bea1d346cf 100644 --- a/net/netfilter/xt_TPROXY.c +++ b/net/netfilter/xt_TPROXY.c @@ -74,18 +74,10 @@ tproxy_tg4(struct net *net, struct sk_buff *skb, __be32 laddr, __be16 lport, /* This should be in a separate target, but we don't do multiple targets on the same rule yet */ skb->mark = (skb->mark & ~mark_mask) ^ mark_value; - - pr_debug("redirecting: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", - iph->protocol, &iph->daddr, ntohs(hp->dest), - &laddr, ntohs(lport), skb->mark); - nf_tproxy_assign_sock(skb, sk); return NF_ACCEPT; } - pr_debug("no socket, dropping: proto %hhu %pI4:%hu -> %pI4:%hu, mark: %x\n", - iph->protocol, &iph->saddr, ntohs(hp->source), - &iph->daddr, ntohs(hp->dest), skb->mark); return NF_DROP; } @@ -122,16 +114,12 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) int tproto; tproto = ipv6_find_hdr(skb, &thoff, -1, NULL, NULL); - if (tproto < 0) { - pr_debug("unable to find transport header in IPv6 packet, dropping\n"); + if (tproto < 0) return NF_DROP; - } hp = skb_header_pointer(skb, thoff, sizeof(_hdr), &_hdr); - if (hp == NULL) { - pr_debug("unable to grab transport header contents in IPv6 packet, dropping\n"); + if (!hp) return NF_DROP; - } /* check if there's an ongoing connection on the packet * addresses, this happens if the redirect already happened @@ -168,19 +156,10 @@ tproxy_tg6_v1(struct sk_buff *skb, const struct xt_action_param *par) /* This should be in a separate target, but we don't do multiple targets on the same rule yet */ skb->mark = (skb->mark & ~tgi->mark_mask) ^ tgi->mark_value; - - pr_debug("redirecting: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", - tproto, &iph->saddr, ntohs(hp->source), - laddr, ntohs(lport), skb->mark); - nf_tproxy_assign_sock(skb, sk); return NF_ACCEPT; } - pr_debug("no socket, dropping: proto %hhu %pI6:%hu -> %pI6:%hu, mark: %x\n", - tproto, &iph->saddr, ntohs(hp->source), - &iph->daddr, ntohs(hp->dest), skb->mark); - return NF_DROP; } -- 2.37.0.144.g8ac04bfd2-goog ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-12 20:49 ` [PATCH v2] " Justin Stitt @ 2022-07-18 17:43 ` Justin Stitt 2022-07-18 21:38 ` Pablo Neira Ayuso 0 siblings, 1 reply; 11+ messages in thread From: Justin Stitt @ 2022-07-18 17:43 UTC (permalink / raw) To: nathan Cc: coreteam, davem, edumazet, fw, kadlec, kuba, linux-kernel, llvm, ndesaulniers, netdev, netfilter-devel, pabeni, pablo, trix Any chance a maintainer could take a look at this patch? I am trying to get it through this cycle and we are so close to enabling the -Wformat option for Clang. There's only a handful of patches remaining until the patch enabling this warning can be sent! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] netfilter: xt_TPROXY: remove pr_debug invocations 2022-07-18 17:43 ` Justin Stitt @ 2022-07-18 21:38 ` Pablo Neira Ayuso 0 siblings, 0 replies; 11+ messages in thread From: Pablo Neira Ayuso @ 2022-07-18 21:38 UTC (permalink / raw) To: Justin Stitt Cc: nathan, coreteam, davem, edumazet, fw, kadlec, kuba, linux-kernel, llvm, ndesaulniers, netdev, netfilter-devel, pabeni, trix On Mon, Jul 18, 2022 at 10:43:17AM -0700, Justin Stitt wrote: > Any chance a maintainer could take a look at this patch? I am trying > to get it through this cycle and we are so close to enabling the > -Wformat option for Clang. There's only a handful of patches remaining > until the patch enabling this warning can be sent! I'll place this into nf-next, thanks. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-07-18 21:38 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-07-07 19:17 [PATCH] netfilter: xt_TPROXY: fix clang -Wformat warnings: Justin Stitt 2022-07-08 23:33 ` Nick Desaulniers 2022-07-11 9:04 ` Pablo Neira Ayuso 2022-07-11 19:44 ` Justin Stitt 2022-07-12 8:40 ` Pablo Neira Ayuso 2022-07-12 18:34 ` [PATCH] netfilter: xt_TPROXY: remove pr_debug invocations Justin Stitt 2022-07-12 18:56 ` Nathan Chancellor 2022-07-12 20:38 ` Justin Stitt 2022-07-12 20:49 ` [PATCH v2] " Justin Stitt 2022-07-18 17:43 ` Justin Stitt 2022-07-18 21:38 ` Pablo Neira Ayuso
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).