Re: [PATCH v2 net-next 2/2] net: socket: change MSG_CMSG_COMPAT to BIT(21)

2021-03-21 Thread Guenter Roeck
On 3/21/21 6:43 AM, menglong8.d...@gmail.com wrote:
> From: Menglong Dong 
> 
> Currently, MSG_CMSG_COMPAT is defined as '1 << 31'. However, 'msg_flags'
> is defined with type of 'int' somewhere, such as 'packet_recvmsg' and
> other recvmsg functions:
> 
> static int packet_recvmsg(struct socket *sock, struct msghdr *msg,
> size_t len,
> int flags)
> 
> If MSG_CMSG_COMPAT is set in 'flags', it's value will be negative.
> Once it perform bit operations with MSG_*, the upper 32 bits of
> the result will be set, just like what Guenter Roeck explained
> here:
> 
> https://lore.kernel.org/netdev/20210317013758.ga134...@roeck-us.net
> 
> As David Laight suggested, fix this by change MSG_CMSG_COMPAT to
> some value else. MSG_CMSG_COMPAT is an internal value, which is't
> used in userspace, so this change works.
> 

Maybe I am overly concerned (or maybe call it pessimistic),
but I do wonder if this change is worth the added risk. Personally
I'd rather start changing all 'int flag' uses to 'unsigned int flag'
and, only after that is complete, make the switch to BIT().

Of course, that is just my personal opinion, and, as I said,
I may be overly concerned.

Guenter

> Reported-by: Guenter Roeck 
> Signed-off-by: Menglong Dong 
> ---
> v2:
> - add comment to stop people from trying to use BIT(31)
> ---
>  include/linux/socket.h | 14 +-
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index d5ebfe30d96b..61b2694d68dd 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -312,17 +312,21 @@ struct ucred {
>* plain text and require encryption
>*/
>  
> +#if defined(CONFIG_COMPAT)
> +#define MSG_CMSG_COMPAT  BIT(21) /* This message needs 32 bit 
> fixups */
> +#else
> +#define MSG_CMSG_COMPAT  0   /* We never have 32 bit fixups 
> */
> +#endif
> +
>  #define MSG_ZEROCOPY BIT(26) /* Use user data in kernel path */
>  #define MSG_FASTOPEN BIT(29) /* Send data in TCP SYN */
>  #define MSG_CMSG_CLOEXEC BIT(30) /* Set close_on_exec for file
>* descriptor received through
>* SCM_RIGHTS
>*/
> -#if defined(CONFIG_COMPAT)
> -#define MSG_CMSG_COMPAT  BIT(31) /* This message needs 32 bit 
> fixups */
> -#else
> -#define MSG_CMSG_COMPAT  0   /* We never have 32 bit fixups 
> */
> -#endif
> +/* Attention: Don't use BIT(31) for MSG_*, as 'msg_flags' is defined
> + * as 'int' somewhere and BIT(31) will make it become negative.
> + */
>  
>  
>  /* Setsockoptions(2) level. Thanks to BSD these must match IPPROTO_xxx */
> 



[PATCH v2 net-next 2/2] net: socket: change MSG_CMSG_COMPAT to BIT(21)

2021-03-21 Thread menglong8 . dong
From: Menglong Dong 

Currently, MSG_CMSG_COMPAT is defined as '1 << 31'. However, 'msg_flags'
is defined with type of 'int' somewhere, such as 'packet_recvmsg' and
other recvmsg functions:

static int packet_recvmsg(struct socket *sock, struct msghdr *msg,
  size_t len,
  int flags)

If MSG_CMSG_COMPAT is set in 'flags', it's value will be negative.
Once it perform bit operations with MSG_*, the upper 32 bits of
the result will be set, just like what Guenter Roeck explained
here:

https://lore.kernel.org/netdev/20210317013758.ga134...@roeck-us.net

As David Laight suggested, fix this by change MSG_CMSG_COMPAT to
some value else. MSG_CMSG_COMPAT is an internal value, which is't
used in userspace, so this change works.

Reported-by: Guenter Roeck 
Signed-off-by: Menglong Dong 
---
v2:
- add comment to stop people from trying to use BIT(31)
---
 include/linux/socket.h | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/socket.h b/include/linux/socket.h
index d5ebfe30d96b..61b2694d68dd 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -312,17 +312,21 @@ struct ucred {
 * plain text and require encryption
 */
 
+#if defined(CONFIG_COMPAT)
+#define MSG_CMSG_COMPATBIT(21) /* This message needs 32 bit 
fixups */
+#else
+#define MSG_CMSG_COMPAT0   /* We never have 32 bit fixups 
*/
+#endif
+
 #define MSG_ZEROCOPY   BIT(26) /* Use user data in kernel path */
 #define MSG_FASTOPEN   BIT(29) /* Send data in TCP SYN */
 #define MSG_CMSG_CLOEXEC   BIT(30) /* Set close_on_exec for file
 * descriptor received through
 * SCM_RIGHTS
 */
-#if defined(CONFIG_COMPAT)
-#define MSG_CMSG_COMPATBIT(31) /* This message needs 32 bit 
fixups */
-#else
-#define MSG_CMSG_COMPAT0   /* We never have 32 bit fixups 
*/
-#endif
+/* Attention: Don't use BIT(31) for MSG_*, as 'msg_flags' is defined
+ * as 'int' somewhere and BIT(31) will make it become negative.
+ */
 
 
 /* Setsockoptions(2) level. Thanks to BSD these must match IPPROTO_xxx */
-- 
2.31.0