[PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Stephen Kitt]

Commit c7228317441f ("net: Use a more standard macro for
INET_ADDR_COOKIE") added a __deprecated marker to the cookie name on
32-bit architectures, with the intent that the compiler would flag
uses of the name. However since commit 771c035372a0 ("deprecate the
'__deprecated' attribute warnings entirely and for good"),
__deprecated doesn't do anything and should be avoided.

This patch changes INET_ADDR_COOKIE to declare a dummy struct so that
any subsequent use of the cookie's name will in all likelihood break
the build. It also removes the __deprecated marker.

Signed-off-by: Stephen Kitt <steve@sk2.org>
---
Changes since v1:
  - use a dummy struct rather than a typedef

 include/net/inet_hashtables.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
index ad64ba6a057f..889d9b00c905 100644
--- a/include/net/inet_hashtables.h
+++ b/include/net/inet_hashtables.h
@@ -301,8 +301,9 @@ static inline struct sock *inet_lookup_listener(struct net *net,
 	  ((__sk)->sk_bound_dev_if == (__sdif)))		&&	\
 	 net_eq(sock_net(__sk), (__net)))
 #else /* 32-bit arch */
+/* Break the build if anything tries to use the cookie's name. */
 #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
-	const int __name __deprecated __attribute__((unused))
+	struct {} __name __attribute__((unused))
 
 #define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
 	(((__sk)->sk_portpair == (__ports))		&&		\
-- 
2.20.1

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Jakub Kicinski]

On Fri,  8 May 2020 14:04:57 +0200 Stephen Kitt wrote:
> Commit c7228317441f ("net: Use a more standard macro for
> INET_ADDR_COOKIE") added a __deprecated marker to the cookie name on
> 32-bit architectures, with the intent that the compiler would flag
> uses of the name. However since commit 771c035372a0 ("deprecate the
> '__deprecated' attribute warnings entirely and for good"),
> __deprecated doesn't do anything and should be avoided.
> 
> This patch changes INET_ADDR_COOKIE to declare a dummy struct so that
> any subsequent use of the cookie's name will in all likelihood break
> the build. It also removes the __deprecated marker.
> 
> Signed-off-by: Stephen Kitt <steve@sk2.org>
> ---
> Changes since v1:
>   - use a dummy struct rather than a typedef
> 
>  include/net/inet_hashtables.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/net/inet_hashtables.h b/include/net/inet_hashtables.h
> index ad64ba6a057f..889d9b00c905 100644
> --- a/include/net/inet_hashtables.h
> +++ b/include/net/inet_hashtables.h
> @@ -301,8 +301,9 @@ static inline struct sock *inet_lookup_listener(struct net *net,
>  	  ((__sk)->sk_bound_dev_if == (__sdif)))		&&	\
>  	 net_eq(sock_net(__sk), (__net)))
>  #else /* 32-bit arch */
> +/* Break the build if anything tries to use the cookie's name. */

I think the macro is supposed to cause a warning when the variable
itself is accessed. And I don't think that happens with your patch
applied.

diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 2bbaaf0c7176..6c4a3904ed8b 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -360,6 +360,8 @@ struct sock *__inet_lookup_established(struct net *net,
        unsigned int slot = hash & hashinfo->ehash_mask;
        struct inet_ehash_bucket *head = &hashinfo->ehash[slot];
 
+       kfree(&acookie);
 begin:
        sk_nulls_for_each_rcu(sk, node, &head->chain) {
                if (sk->sk_hash != hash)

$ make ARCH=i386
make[1]: Entering directory `/netdev/net-next/build_allmodconfig_warn_32bit'
  GEN     Makefile
  CALL    ../scripts/atomic/check-atomics.sh
  CALL    ../scripts/checksyscalls.sh
  CHK     include/generated/compile.h
  CC      net/ipv4/inet_hashtables.o
  CHK     kernel/kheaders_data.tar.xz
  AR      net/ipv4/built-in.a
  AR      net/built-in.a
  GEN     .version
  CHK     include/generated/compile.h
  UPD     include/generated/compile.h
  CC      init/version.o
  AR      init/built-in.a
  LD      vmlinux.o
  MODPOST vmlinux.o

Builds fine.

>  #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> -	const int __name __deprecated __attribute__((unused))
> +	struct {} __name __attribute__((unused))
>  
>  #define INET_MATCH(__sk, __net, __cookie, __saddr, __daddr, __ports, __dif, __sdif) \
>  	(((__sk)->sk_portpair == (__ports))		&&		\

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

Hi,

Thanks for taking the time to review my patch.

On Fri, 8 May 2020 20:50:25 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Fri,  8 May 2020 14:04:57 +0200 Stephen Kitt wrote:
> > Commit c7228317441f ("net: Use a more standard macro for
> > INET_ADDR_COOKIE") added a __deprecated marker to the cookie name on
> > 32-bit architectures, with the intent that the compiler would flag
> > uses of the name. However since commit 771c035372a0 ("deprecate the
> > '__deprecated' attribute warnings entirely and for good"),
> > __deprecated doesn't do anything and should be avoided.
> > 
> > This patch changes INET_ADDR_COOKIE to declare a dummy struct so that
> > any subsequent use of the cookie's name will in all likelihood break
> > the build. It also removes the __deprecated marker.
> 
> I think the macro is supposed to cause a warning when the variable
> itself is accessed. And I don't think that happens with your patch
> applied.

Yes, the warning is what was lost when __deprecated lost its meaning. I was
trying to preserve that, or rather extend it so that the build would break if
the cookie was used on 32-bit architectures, and my patch ensures it does if
the cookie is used in a comparison or assignment, but ...

> +       kfree(&acookie);

I hadn’t thought of taking a pointer to it.

If we want to preserve the use of the macro with a semi-colon, which is what
Joe’s patch introduced (along with the deprecation warning), we still need
some sort of declaration which can’t be used. Perhaps

#define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
	struct __name {} __attribute__((unused))

would be better — it declares the cookie as a struct, not a variable, so then
the build fails if the cookie is used as anything other than a struct. If
anyone does try to use it as a struct, the build will fail on 64-bit
architectures...

  CC      net/ipv4/inet_hashtables.o
net/ipv4/inet_hashtables.c: In function ‘__inet_lookup_established’:
net/ipv4/inet_hashtables.c:362:9: error: ‘acookie’ undeclared (first use in this function)
  kfree(&acookie);
         ^~~~~~~
net/ipv4/inet_hashtables.c:362:9: note: each undeclared identifier is reported only once for each function it appears in
make[2]: *** [scripts/Makefile.build:267: net/ipv4/inet_hashtables.o] Error 1
make[1]: *** [scripts/Makefile.build:488: net/ipv4] Error 2
make: *** Makefile:1722: net] Error 2

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Jakub Kicinski]

On Sat, 9 May 2020 10:13:22 +0200 Stephen Kitt wrote:
> Hi,
> 
> Thanks for taking the time to review my patch.
> 
> On Fri, 8 May 2020 20:50:25 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> > On Fri,  8 May 2020 14:04:57 +0200 Stephen Kitt wrote:  
> > > Commit c7228317441f ("net: Use a more standard macro for
> > > INET_ADDR_COOKIE") added a __deprecated marker to the cookie name on
> > > 32-bit architectures, with the intent that the compiler would flag
> > > uses of the name. However since commit 771c035372a0 ("deprecate the
> > > '__deprecated' attribute warnings entirely and for good"),
> > > __deprecated doesn't do anything and should be avoided.
> > > 
> > > This patch changes INET_ADDR_COOKIE to declare a dummy struct so that
> > > any subsequent use of the cookie's name will in all likelihood break
> > > the build. It also removes the __deprecated marker.  
> > 
> > I think the macro is supposed to cause a warning when the variable
> > itself is accessed. And I don't think that happens with your patch
> > applied.  
> 
> Yes, the warning is what was lost when __deprecated lost its meaning. I was
> trying to preserve that, or rather extend it so that the build would break if
> the cookie was used on 32-bit architectures, and my patch ensures it does if
> the cookie is used in a comparison or assignment, but ...
> 
> > +       kfree(&acookie);  
> 
> I hadn’t thought of taking a pointer to it.
> 
> If we want to preserve the use of the macro with a semi-colon, which is what
> Joe’s patch introduced (along with the deprecation warning), we still need
> some sort of declaration which can’t be used. Perhaps
> 
> #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> 	struct __name {} __attribute__((unused))
> 
> would be better — it declares the cookie as a struct, not a variable, so then
> the build fails if the cookie is used as anything other than a struct. If
> anyone does try to use it as a struct, the build will fail on 64-bit
> architectures...
> 
>   CC      net/ipv4/inet_hashtables.o
> net/ipv4/inet_hashtables.c: In function ‘__inet_lookup_established’:
> net/ipv4/inet_hashtables.c:362:9: error: ‘acookie’ undeclared (first use in this function)
>   kfree(&acookie);
>          ^~~~~~~
> net/ipv4/inet_hashtables.c:362:9: note: each undeclared identifier is reported only once for each function it appears in
> make[2]: *** [scripts/Makefile.build:267: net/ipv4/inet_hashtables.o] Error 1
> make[1]: *** [scripts/Makefile.build:488: net/ipv4] Error 2
> make: *** Makefile:1722: net] Error 2

Hm. That does seem better. Although thinking about it - we will not get
a warning when someone declares a variable with the same name..

What if we went back to your original proposal of an empty struct but
added in an extern in front? That way we should get linker error on
pointer references.

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 3440 bytes --]

On Sat, 9 May 2020 10:59:14 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> On Sat, 9 May 2020 10:13:22 +0200 Stephen Kitt wrote:
> > On Fri, 8 May 2020 20:50:25 -0700, Jakub Kicinski <kuba@kernel.org>
> > wrote:  
> > > On Fri,  8 May 2020 14:04:57 +0200 Stephen Kitt wrote:    
> > > > Commit c7228317441f ("net: Use a more standard macro for
> > > > INET_ADDR_COOKIE") added a __deprecated marker to the cookie name on
> > > > 32-bit architectures, with the intent that the compiler would flag
> > > > uses of the name. However since commit 771c035372a0 ("deprecate the
> > > > '__deprecated' attribute warnings entirely and for good"),
> > > > __deprecated doesn't do anything and should be avoided.
> > > > 
> > > > This patch changes INET_ADDR_COOKIE to declare a dummy struct so that
> > > > any subsequent use of the cookie's name will in all likelihood break
> > > > the build. It also removes the __deprecated marker.    
> > > 
> > > I think the macro is supposed to cause a warning when the variable
> > > itself is accessed. And I don't think that happens with your patch
> > > applied.    
> > 
> > Yes, the warning is what was lost when __deprecated lost its meaning. I
> > was trying to preserve that, or rather extend it so that the build would
> > break if the cookie was used on 32-bit architectures, and my patch
> > ensures it does if the cookie is used in a comparison or assignment,
> > but ... 
> > > +       kfree(&acookie);    
> > 
> > I hadn’t thought of taking a pointer to it.
> > 
> > If we want to preserve the use of the macro with a semi-colon, which is
> > what Joe’s patch introduced (along with the deprecation warning), we
> > still need some sort of declaration which can’t be used. Perhaps
> > 
> > #define INET_ADDR_COOKIE(__name, __saddr, __daddr) \
> > 	struct __name {} __attribute__((unused))
> > 
> > would be better — it declares the cookie as a struct, not a variable, so
> > then the build fails if the cookie is used as anything other than a
> > struct. If anyone does try to use it as a struct, the build will fail on
> > 64-bit architectures...
> > 
> >   CC      net/ipv4/inet_hashtables.o
> > net/ipv4/inet_hashtables.c: In function ‘__inet_lookup_established’:
> > net/ipv4/inet_hashtables.c:362:9: error: ‘acookie’ undeclared (first use
> > in this function) kfree(&acookie);
> >          ^~~~~~~
> > net/ipv4/inet_hashtables.c:362:9: note: each undeclared identifier is
> > reported only once for each function it appears in make[2]: ***
> > [scripts/Makefile.build:267: net/ipv4/inet_hashtables.o] Error 1 make[1]:
> > *** [scripts/Makefile.build:488: net/ipv4] Error 2 make: ***
> > Makefile:1722: net] Error 2  
> 
> Hm. That does seem better. Although thinking about it - we will not get
> a warning when someone declares a variable with the same name..

Good point!

> What if we went back to your original proposal of an empty struct but
> added in an extern in front? That way we should get linker error on
> pointer references.

That silently fails to fail if any other link object provides a definition
for the symbol, even if the type doesn’t match...

I thought of

	register struct {} __name __attribute__((unused))

but that really feels like tacking on more stuff to handle cases as we think
of them, which makes me wonder what cases I’m not thinking of.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Stephen Kitt]

[-- Attachment #1: Type: text/plain, Size: 592 bytes --]

On Sat, 9 May 2020 21:05:48 +0200, Stephen Kitt <steve@sk2.org> wrote:
> On Sat, 9 May 2020 10:59:14 -0700, Jakub Kicinski <kuba@kernel.org> wrote:
> > What if we went back to your original proposal of an empty struct but
> > added in an extern in front? That way we should get linker error on
> > pointer references.  
> 
> That silently fails to fail if any other link object provides a definition
> for the symbol, even if the type doesn’t match...

And it breaks the build if INET_ADDR_COOKIE is used twice in the same unit,
e.g. in inet_hashtables.c.

Regards,

Stephen

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] net: Protect INET_ADDR_COOKIE on 32-bit architectures

[Jakub Kicinski]

On Sat, 9 May 2020 22:49:28 +0200 Stephen Kitt wrote:
> On Sat, 9 May 2020 21:05:48 +0200, Stephen Kitt <steve@sk2.org> wrote:
> > On Sat, 9 May 2020 10:59:14 -0700, Jakub Kicinski <kuba@kernel.org> wrote:  
> > > What if we went back to your original proposal of an empty struct but
> > > added in an extern in front? That way we should get linker error on
> > > pointer references.    
> > 
> > That silently fails to fail if any other link object provides a definition
> > for the symbol, even if the type doesn’t match...  
> 
> And it breaks the build if INET_ADDR_COOKIE is used twice in the same unit,
> e.g. in inet_hashtables.c.

Ah, so we'd have to use a valid type like, say, char.