2022-11-10 10:26:53

by David Howells

Subject: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
on IPV6 support being enabled.

Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
Reported-by: kernel test robot <[email protected]>
Signed-off-by: David Howells <[email protected]>
cc: Marc Dionne <[email protected]>
cc: [email protected]
cc: [email protected]
---

net/rxrpc/local_object.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index a178f71e5082..25cdfcf7b415 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -33,7 +33,9 @@ static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
{
if (ip_hdr(skb)->version == IPVERSION)
return ip_icmp_error(sk, skb, err, port, info, payload);
+#ifdef CONFIG_AF_RXRPC_IPV6
return ipv6_icmp_error(sk, skb, err, port, info, payload);
+#endif
}

/*

2022-11-10 12:08:29

by Hideaki Yoshifuji

Subject: Re: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

Hi,

2022年11月10日(木) 18:44 David Howells <[email protected]>:
>
> Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
> on IPV6 support being enabled.
>
> Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> cc: Marc Dionne <[email protected]>
> cc: [email protected]
> cc: [email protected]
> ---
>
> net/rxrpc/local_object.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
> index a178f71e5082..25cdfcf7b415 100644
> --- a/net/rxrpc/local_object.c
> +++ b/net/rxrpc/local_object.c
> @@ -33,7 +33,9 @@ static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
> {
> if (ip_hdr(skb)->version == IPVERSION)
> return ip_icmp_error(sk, skb, err, port, info, payload);
> +#ifdef CONFIG_AF_RXRPC_IPV6
> return ipv6_icmp_error(sk, skb, err, port, info, payload);
> +#endif
> }
>

Because this introduces a missing return literally without CONFIG_AF_RXRPC_IPV6,
I would prefer #ifdef / #else for the whole function.

--yoshfuji

> /*
>
>

2022-11-10 13:34:56

by David Howells

Subject: Re: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

Hideaki Yoshifuji <[email protected]> wrote:

> Because this introduces a missing return literally without
> CONFIG_AF_RXRPC_IPV6, I would prefer #ifdef / #else for the whole function.

They're both void functions, so actually, there's nothing to return.

David

2022-11-10 14:33:23

by Andrew Lunn

Subject: Re: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

On Thu, Nov 10, 2022 at 09:43:34AM +0000, David Howells wrote:
> Fix rxrpc_encap_err_rcv() to make the call to ipv6_icmp_error conditional
> on IPV6 support being enabled.
>
> Fixes: b6c66c4324e7 ("rxrpc: Use the core ICMP/ICMP6 parsers")
> Reported-by: kernel test robot <[email protected]>
> Signed-off-by: David Howells <[email protected]>
> cc: Marc Dionne <[email protected]>
> cc: [email protected]
> cc: [email protected]
> ---
>
> net/rxrpc/local_object.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
> index a178f71e5082..25cdfcf7b415 100644
> --- a/net/rxrpc/local_object.c
> +++ b/net/rxrpc/local_object.c
> @@ -33,7 +33,9 @@ static void rxrpc_encap_err_rcv(struct sock *sk, struct sk_buff *skb, int err,
> {
> if (ip_hdr(skb)->version == IPVERSION)
> return ip_icmp_error(sk, skb, err, port, info, payload);
> +#ifdef CONFIG_AF_RXRPC_IPV6
> return ipv6_icmp_error(sk, skb, err, port, info, payload);
> +#endif

Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
#ifdef? It gives better build testing.

Andrew

2022-11-10 15:27:05

by David Howells

Subject: Re: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

Andrew Lunn <[email protected]> wrote:

> > +#ifdef CONFIG_AF_RXRPC_IPV6
> > return ipv6_icmp_error(sk, skb, err, port, info, payload);
> > +#endif
>
> Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
> #ifdef? It gives better build testing.

Sure. Does it actually make that much of a difference? I guess the
declaration is there even if IPV6 is disabled.

David

2022-11-10 16:08:35

by Andrew Lunn

Subject: Re: [PATCH net-next] rxrpc: Fix missing IPV6 #ifdef

On Thu, Nov 10, 2022 at 02:09:45PM +0000, David Howells wrote:
> Andrew Lunn <[email protected]> wrote:
>
> > > +#ifdef CONFIG_AF_RXRPC_IPV6
> > > return ipv6_icmp_error(sk, skb, err, port, info, payload);
> > > +#endif
> >
> > Can this be if (IS_ENABLED(CONFIG_AF_RXRPC_IPV6) {} rather than
> > #ifdef? It gives better build testing.
>
> Sure. Does it actually make that much of a difference? I guess the
> declaration is there even if IPV6 is disabled.

And that is the point. Even if this feature is disabled, the type
checking will be performed, the number of parameters etc. Somebody who
modifies ipv6_icmp_error() is going to find problems with a single
compilation, rather than having to use a big collection of random
configs.

So this is a review comment i often make. Avoid #ifdef if you can, use
IS_ENABLED() inside an if statement.

Andrew