Re: [PATCH v2] netfilter: conntrack: Fix clang -Wformat warning in print_tuple()

[Nick Desaulniers <ndesaulniers@google.com>]

On Tue, Jun 7, 2022 at 11:09 AM Justin Stitt <jstitt007@gmail.com> wrote:
>
>  | net/netfilter/nf_conntrack_standalone.c:63:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->src.u.tcp.port),
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:64:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->dst.u.tcp.port));
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:69:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->src.u.udp.port),
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:70:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->dst.u.udp.port));
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:75:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->src.u.dccp.port),
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:76:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->dst.u.dccp.port));
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:80:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->src.u.sctp.port),
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  | net/netfilter/nf_conntrack_standalone.c:81:7: warning: format specifies type
>  | 'unsigned short' but the argument has type 'int' [-Wformat]
>  |                            ntohs(tuple->dst.u.sctp.port));
>  |                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> Variadic functions (printf-like) undergo default argument promotion.
> Documentation/core-api/printk-formats.rst specifically recommends
> using the promoted-to-type's format flag.
>
> Also, as per C11 6.3.1.1:
> (https://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf)
> `If an int can represent all values of the original type ..., the
> value is converted to an int; otherwise, it is converted to an
> unsigned int. These are called the integer promotions.`
> Thus it makes sense to change %hu (as well as %u) to %d.
>
> It should be noted that %u does not produce the same warning as %hu in this
> context. However, it should probably be changed as well for consistency.

Right, because they are `unsigned char` and the parameter is unnamed
for variadic functions they are also default-argument-promoted to int.
-Wformat won't warn on signedness.

Thanks for the patch!

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/378

Also, Nathan supplied his RB tag on v1; it's ok next time to include
it on subsequent revisions of patches, so long as you don't change the
patch too much between revisions.

>
> Signed-off-by: Justin Stitt <jstitt007@gmail.com>
> ---
>  Diff between v1 -> v2:
>  * update commit message and subject line
>
>  Note: The architecture (arm64) is critical for reproducing this warning.
>
>  net/netfilter/nf_conntrack_standalone.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/nf_conntrack_standalone.c b/net/netfilter/nf_conntrack_standalone.c
> index 6ad7bbc90d38..afbec8a12c5e 100644
> --- a/net/netfilter/nf_conntrack_standalone.c
> +++ b/net/netfilter/nf_conntrack_standalone.c
> @@ -53,30 +53,30 @@ print_tuple(struct seq_file *s, const struct nf_conntrack_tuple *tuple,
>
>         switch (l4proto->l4proto) {
>         case IPPROTO_ICMP:
> -               seq_printf(s, "type=%u code=%u id=%u ",
> +               seq_printf(s, "type=%d code=%d id=%d ",
>                            tuple->dst.u.icmp.type,
>                            tuple->dst.u.icmp.code,
>                            ntohs(tuple->src.u.icmp.id));
>                 break;
>         case IPPROTO_TCP:
> -               seq_printf(s, "sport=%hu dport=%hu ",
> +               seq_printf(s, "sport=%d dport=%d ",
>                            ntohs(tuple->src.u.tcp.port),
>                            ntohs(tuple->dst.u.tcp.port));
>                 break;
>         case IPPROTO_UDPLITE:
>         case IPPROTO_UDP:
> -               seq_printf(s, "sport=%hu dport=%hu ",
> +               seq_printf(s, "sport=%d dport=%d ",
>                            ntohs(tuple->src.u.udp.port),
>                            ntohs(tuple->dst.u.udp.port));
>
>                 break;
>         case IPPROTO_DCCP:
> -               seq_printf(s, "sport=%hu dport=%hu ",
> +               seq_printf(s, "sport=%d dport=%d ",
>                            ntohs(tuple->src.u.dccp.port),
>                            ntohs(tuple->dst.u.dccp.port));
>                 break;
>         case IPPROTO_SCTP:
> -               seq_printf(s, "sport=%hu dport=%hu ",
> +               seq_printf(s, "sport=%d dport=%d ",
>                            ntohs(tuple->src.u.sctp.port),
>                            ntohs(tuple->dst.u.sctp.port));
>                 break;
> --
> 2.30.2
>


-- 
Thanks,
~Nick Desaulniers