linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
@ 2021-03-10  1:51 menglong8.dong
  2021-03-10 21:00 ` patchwork-bot+netdevbpf
  2021-03-16 22:48 ` Guenter Roeck
  0 siblings, 2 replies; 15+ messages in thread
From: menglong8.dong @ 2021-03-10  1:51 UTC (permalink / raw)
  To: kuba, andy.shevchenko
  Cc: davem, axboe, viro, herbert, dong.menglong, linux-kernel, netdev

From: Menglong Dong <dong.menglong@zte.com.cn>

The bit mask for MSG_* seems a little confused here. Replace it
with BIT() to make it clear to understand.

Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn>
---
v4:
- CC netdev
v3:
- move changelog here
v2:
- use BIT() instead of BIT_MASK()
---
 include/linux/socket.h | 71 ++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 34 deletions(-)

diff --git a/include/linux/socket.h b/include/linux/socket.h
index 385894b4a8bb..e88859f38cd0 100644
--- a/include/linux/socket.h
+++ b/include/linux/socket.h
@@ -283,42 +283,45 @@ struct ucred {
    Added those for 1003.1g not all are supported yet
  */
 
-#define MSG_OOB		1
-#define MSG_PEEK	2
-#define MSG_DONTROUTE	4
-#define MSG_TRYHARD     4       /* Synonym for MSG_DONTROUTE for DECnet */
-#define MSG_CTRUNC	8
-#define MSG_PROBE	0x10	/* Do not send. Only probe path f.e. for MTU */
-#define MSG_TRUNC	0x20
-#define MSG_DONTWAIT	0x40	/* Nonblocking io		 */
-#define MSG_EOR         0x80	/* End of record */
-#define MSG_WAITALL	0x100	/* Wait for a full request */
-#define MSG_FIN         0x200
-#define MSG_SYN		0x400
-#define MSG_CONFIRM	0x800	/* Confirm path validity */
-#define MSG_RST		0x1000
-#define MSG_ERRQUEUE	0x2000	/* Fetch message from error queue */
-#define MSG_NOSIGNAL	0x4000	/* Do not generate SIGPIPE */
-#define MSG_MORE	0x8000	/* Sender will send more */
-#define MSG_WAITFORONE	0x10000	/* recvmmsg(): block until 1+ packets avail */
-#define MSG_SENDPAGE_NOPOLICY 0x10000 /* sendpage() internal : do no apply policy */
-#define MSG_SENDPAGE_NOTLAST 0x20000 /* sendpage() internal : not the last page */
-#define MSG_BATCH	0x40000 /* sendmmsg(): more messages coming */
-#define MSG_EOF         MSG_FIN
-#define MSG_NO_SHARED_FRAGS 0x80000 /* sendpage() internal : page frags are not shared */
-#define MSG_SENDPAGE_DECRYPTED	0x100000 /* sendpage() internal : page may carry
-					  * plain text and require encryption
-					  */
-
-#define MSG_ZEROCOPY	0x4000000	/* Use user data in kernel path */
-#define MSG_FASTOPEN	0x20000000	/* Send data in TCP SYN */
-#define MSG_CMSG_CLOEXEC 0x40000000	/* Set close_on_exec for file
-					   descriptor received through
-					   SCM_RIGHTS */
+#define MSG_OOB		BIT(0)
+#define MSG_PEEK	BIT(1)
+#define MSG_DONTROUTE	BIT(2)
+#define MSG_TRYHARD	BIT(2)	/* Synonym for MSG_DONTROUTE for DECnet		*/
+#define MSG_CTRUNC	BIT(3)
+#define MSG_PROBE	BIT(4)	/* Do not send. Only probe path f.e. for MTU	*/
+#define MSG_TRUNC	BIT(5)
+#define MSG_DONTWAIT	BIT(6)	/* Nonblocking io		*/
+#define MSG_EOR		BIT(7)	/* End of record		*/
+#define MSG_WAITALL	BIT(8)	/* Wait for a full request	*/
+#define MSG_FIN		BIT(9)
+#define MSG_SYN		BIT(10)
+#define MSG_CONFIRM	BIT(11)	/* Confirm path validity	*/
+#define MSG_RST		BIT(12)
+#define MSG_ERRQUEUE	BIT(13)	/* Fetch message from error queue */
+#define MSG_NOSIGNAL	BIT(14)	/* Do not generate SIGPIPE	*/
+#define MSG_MORE	BIT(15)	/* Sender will send more	*/
+#define MSG_WAITFORONE	BIT(16)	/* recvmmsg(): block until 1+ packets avail */
+#define MSG_SENDPAGE_NOPOLICY	BIT(16)	/* sendpage() internal : do no apply policy */
+#define MSG_SENDPAGE_NOTLAST	BIT(17)	/* sendpage() internal : not the last page  */
+#define MSG_BATCH	BIT(18)		/* sendmmsg(): more messages coming */
+#define MSG_EOF		MSG_FIN
+#define MSG_NO_SHARED_FRAGS	BIT(19)	/* sendpage() internal : page frags
+					 * are not shared
+					 */
+#define MSG_SENDPAGE_DECRYPTED	BIT(20)	/* sendpage() internal : page may carry
+					 * plain text and require encryption
+					 */
+
+#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	0x80000000	/* This message needs 32 bit fixups */
+#define MSG_CMSG_COMPAT	BIT(31)	/* This message needs 32 bit fixups */
 #else
-#define MSG_CMSG_COMPAT	0		/* We never have 32 bit fixups */
+#define MSG_CMSG_COMPAT	0	/* We never have 32 bit fixups */
 #endif
 
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-10  1:51 [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_* menglong8.dong
@ 2021-03-10 21:00 ` patchwork-bot+netdevbpf
  2021-03-16 22:48 ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: patchwork-bot+netdevbpf @ 2021-03-10 21:00 UTC (permalink / raw)
  To: Menglong Dong
  Cc: kuba, andy.shevchenko, davem, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Tue,  9 Mar 2021 17:51:35 -0800 you wrote:
> From: Menglong Dong <dong.menglong@zte.com.cn>
> 
> The bit mask for MSG_* seems a little confused here. Replace it
> with BIT() to make it clear to understand.
> 
> Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn>
> 
> [...]

Here is the summary with links:
  - [v4,RESEND,net-next] net: socket: use BIT() for MSG_*
    https://git.kernel.org/netdev/net-next/c/0bb3262c0248

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-10  1:51 [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_* menglong8.dong
  2021-03-10 21:00 ` patchwork-bot+netdevbpf
@ 2021-03-16 22:48 ` Guenter Roeck
       [not found]   ` <CAHp75VdE3fkCjb53vBso5uJX9aEFtAOAdh5NVOSbK0YR64+jOg@mail.gmail.com>
  1 sibling, 1 reply; 15+ messages in thread
From: Guenter Roeck @ 2021-03-16 22:48 UTC (permalink / raw)
  To: menglong8.dong
  Cc: kuba, andy.shevchenko, davem, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

Hi,

On Tue, Mar 09, 2021 at 05:51:35PM -0800, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <dong.menglong@zte.com.cn>
> 
> The bit mask for MSG_* seems a little confused here. Replace it
> with BIT() to make it clear to understand.
> 
> Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn>

I must admit that I am a bit puzzled, but with this patch in the tree
(in next-20210316) several of my qemu network interface tests fail
to get an IP address. So far I have only seen this with mips64
tests, but that may be because I only started running those tests
on various architectures.

The tests do nothing special: With CONFIG_IP_PNP_DHCP=n, run udhcpc
in qemu to get an IP address. With this patch in place, udhcpc fails.
With this patch reverted, udhcpc gets the IP address as expected.
The error reported by udhcpc is:

udhcpc: sending discover
udhcpc: read error: Invalid argument, reopening socket

Reverting this patch fixes the problem.

Guenter

---
bisect log:

# bad: [0f4b0bb396f6f424a7b074d00cb71f5966edcb8a] Add linux-next specific files for 20210316
# good: [1e28eed17697bcf343c6743f0028cc3b5dd88bf0] Linux 5.12-rc3
git bisect start 'HEAD' 'v5.12-rc3'
# bad: [edd84c42baeffe66740143a04f24588fded94241] Merge remote-tracking branch 'drm-misc/for-linux-next'
git bisect bad edd84c42baeffe66740143a04f24588fded94241
# good: [a76f62d56da82bee1a4c35dd6375a8fdd57eca4e] Merge remote-tracking branch 'cel/for-next'
git bisect good a76f62d56da82bee1a4c35dd6375a8fdd57eca4e
# good: [e2924c67bae0cc15ca64dbe1ed791c96eed6d149] Merge remote-tracking branch 'rdma/for-next'
git bisect good e2924c67bae0cc15ca64dbe1ed791c96eed6d149
# bad: [a8f9952d218d816ff1a13c9385edd821a8da527d] selftests: fib_nexthops: List each test case in a different line
git bisect bad a8f9952d218d816ff1a13c9385edd821a8da527d
# bad: [4734a750f4674631ab9896189810b57700597aa7] mlxsw: Adjust some MFDE fields shift and size to fw implementation
git bisect bad 4734a750f4674631ab9896189810b57700597aa7
# good: [32e76b187a90de5809d68c2ef3e3964176dacaf0] bpf: Document BPF_PROG_ATTACH syscall command
git bisect good 32e76b187a90de5809d68c2ef3e3964176dacaf0
# good: [ee75aef23afe6e88497151c127c13ed69f41aaa2] bpf, xdp: Restructure redirect actions
git bisect good ee75aef23afe6e88497151c127c13ed69f41aaa2
# bad: [90d181ca488f466904ea59dd5c836f766b69c71b] net: rose: Fix fall-through warnings for Clang
git bisect bad 90d181ca488f466904ea59dd5c836f766b69c71b
# bad: [537a0c5c4218329990dc8973068f3bfe5c882623] net: fddi: skfp: smt: Replace one-element array with flexible-array member
git bisect bad 537a0c5c4218329990dc8973068f3bfe5c882623
# bad: [97c2c69e1926260c78c7f1c0b2c987934f1dc7a1] virtio-net: support XDP when not more queues
git bisect bad 97c2c69e1926260c78c7f1c0b2c987934f1dc7a1
# good: [c1acda9807e2bbe1d2026b44f37d959d6d8266c8] Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next
git bisect good c1acda9807e2bbe1d2026b44f37d959d6d8266c8
# bad: [0bb3262c0248d44aea3be31076f44beb82a7b120] net: socket: use BIT() for MSG_*
git bisect bad 0bb3262c0248d44aea3be31076f44beb82a7b120
# first bad commit: [0bb3262c0248d44aea3be31076f44beb82a7b120] net: socket: use BIT() for MSG_*

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
       [not found]   ` <CAHp75VdE3fkCjb53vBso5uJX9aEFtAOAdh5NVOSbK0YR64+jOg@mail.gmail.com>
@ 2021-03-16 23:54     ` Guenter Roeck
  2021-03-17  1:37     ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-03-16 23:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: menglong8.dong, kuba, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

On 3/16/21 4:02 PM, Andy Shevchenko wrote:
> 
> 
> On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net <mailto:linux@roeck-us.net>> wrote:
> 
>     Hi,
> 
>     On Tue, Mar 09, 2021 at 05:51:35PM -0800, menglong8.dong@gmail.com <mailto:menglong8.dong@gmail.com> wrote:
>     > From: Menglong Dong <dong.menglong@zte.com.cn <mailto:dong.menglong@zte.com.cn>>
>     >
>     > The bit mask for MSG_* seems a little confused here. Replace it
>     > with BIT() to make it clear to understand.
>     >
>     > Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn <mailto:dong.menglong@zte.com.cn>>
> 
>     I must admit that I am a bit puzzled,
> 
> 
> I have checked the values and don’t see a problem. So, the only difference is the type int vs. unsigned long. I think this simply reveals an issue somewhere in the code.
>  

I am currently trying to "bisect" the individual bits. We'll see if I can find
the culprit(s).

Guenter

> 
> 
>      but with this patch in the tree
>     (in next-20210316) several of my qemu network interface tests fail
>     to get an IP address. So far I have only seen this with mips64
>     tests, but that may be because I only started running those tests
>     on various architectures.
> 
>     The tests do nothing special: With CONFIG_IP_PNP_DHCP=n, run udhcpc
>     in qemu to get an IP address. With this patch in place, udhcpc fails.
>     With this patch reverted, udhcpc gets the IP address as expected.
>     The error reported by udhcpc is:
> 
>     udhcpc: sending discover
>     udhcpc: read error: Invalid argument, reopening socket
> 
>     Reverting this patch fixes the problem.
> 
>     Guenter
> 
>     ---
>     bisect log:
> 
>     # bad: [0f4b0bb396f6f424a7b074d00cb71f5966edcb8a] Add linux-next specific files for 20210316
>     # good: [1e28eed17697bcf343c6743f0028cc3b5dd88bf0] Linux 5.12-rc3
>     git bisect start 'HEAD' 'v5.12-rc3'
>     # bad: [edd84c42baeffe66740143a04f24588fded94241] Merge remote-tracking branch 'drm-misc/for-linux-next'
>     git bisect bad edd84c42baeffe66740143a04f24588fded94241
>     # good: [a76f62d56da82bee1a4c35dd6375a8fdd57eca4e] Merge remote-tracking branch 'cel/for-next'
>     git bisect good a76f62d56da82bee1a4c35dd6375a8fdd57eca4e
>     # good: [e2924c67bae0cc15ca64dbe1ed791c96eed6d149] Merge remote-tracking branch 'rdma/for-next'
>     git bisect good e2924c67bae0cc15ca64dbe1ed791c96eed6d149
>     # bad: [a8f9952d218d816ff1a13c9385edd821a8da527d] selftests: fib_nexthops: List each test case in a different line
>     git bisect bad a8f9952d218d816ff1a13c9385edd821a8da527d
>     # bad: [4734a750f4674631ab9896189810b57700597aa7] mlxsw: Adjust some MFDE fields shift and size to fw implementation
>     git bisect bad 4734a750f4674631ab9896189810b57700597aa7
>     # good: [32e76b187a90de5809d68c2ef3e3964176dacaf0] bpf: Document BPF_PROG_ATTACH syscall command
>     git bisect good 32e76b187a90de5809d68c2ef3e3964176dacaf0
>     # good: [ee75aef23afe6e88497151c127c13ed69f41aaa2] bpf, xdp: Restructure redirect actions
>     git bisect good ee75aef23afe6e88497151c127c13ed69f41aaa2
>     # bad: [90d181ca488f466904ea59dd5c836f766b69c71b] net: rose: Fix fall-through warnings for Clang
>     git bisect bad 90d181ca488f466904ea59dd5c836f766b69c71b
>     # bad: [537a0c5c4218329990dc8973068f3bfe5c882623] net: fddi: skfp: smt: Replace one-element array with flexible-array member
>     git bisect bad 537a0c5c4218329990dc8973068f3bfe5c882623
>     # bad: [97c2c69e1926260c78c7f1c0b2c987934f1dc7a1] virtio-net: support XDP when not more queues
>     git bisect bad 97c2c69e1926260c78c7f1c0b2c987934f1dc7a1
>     # good: [c1acda9807e2bbe1d2026b44f37d959d6d8266c8] Merge git://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next <http://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next>
>     git bisect good c1acda9807e2bbe1d2026b44f37d959d6d8266c8
>     # bad: [0bb3262c0248d44aea3be31076f44beb82a7b120] net: socket: use BIT() for MSG_*
>     git bisect bad 0bb3262c0248d44aea3be31076f44beb82a7b120
>     # first bad commit: [0bb3262c0248d44aea3be31076f44beb82a7b120] net: socket: use BIT() for MSG_*
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
       [not found]   ` <CAHp75VdE3fkCjb53vBso5uJX9aEFtAOAdh5NVOSbK0YR64+jOg@mail.gmail.com>
  2021-03-16 23:54     ` Guenter Roeck
@ 2021-03-17  1:37     ` Guenter Roeck
  2021-03-17  8:21       ` Menglong Dong
  2021-03-17 15:12       ` David Laight
  1 sibling, 2 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-03-17  1:37 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: menglong8.dong, kuba, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net> wrote:
> 
> > Hi,
> >
> > On Tue, Mar 09, 2021 at 05:51:35PM -0800, menglong8.dong@gmail.com wrote:
> > > From: Menglong Dong <dong.menglong@zte.com.cn>
> > >
> > > The bit mask for MSG_* seems a little confused here. Replace it
> > > with BIT() to make it clear to understand.
> > >
> > > Signed-off-by: Menglong Dong <dong.menglong@zte.com.cn>
> >
> > I must admit that I am a bit puzzled,
> 
> 
> I have checked the values and don’t see a problem. So, the only difference
> is the type int vs. unsigned long. I think this simply reveals an issue
> somewhere in the code.
> 
The problem is in net/packet/af_packet.c:packet_recvmsg(). This function,
as well as all other rcvmsg functions, is declared as

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

MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
This is then evaluated in

       if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
                goto out;

If any of those flags is declared as BIT() and thus long, flags is
sign-extended to long. Since it is negative, its upper 32 bits will be set,
the if statement evaluates as true, and the function bails out.

This is relatively easy to fix here with, for example,

        if ((unsigned int)flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
                goto out;

but that is just a hack, and it doesn't solve the real problem:
Each function in struct proto_ops which passes flags passes it as int
(see include/linux/net.h:struct proto_ops). Each such function, if
called with MSG_CMSG_COMPAT set, will fail a match against
~(MSG_anything) if MSG_anything is declared as BIT() or long.

As it turns out, I was kind of lucky to catch the problem: So far I have
seen it only on mips64 kernels with N32 userspace.

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  1:37     ` Guenter Roeck
@ 2021-03-17  8:21       ` Menglong Dong
  2021-03-17  9:36         ` Andy Shevchenko
  2021-03-17 16:39         ` David Miller
  2021-03-17 15:12       ` David Laight
  1 sibling, 2 replies; 15+ messages in thread
From: Menglong Dong @ 2021-03-17  8:21 UTC (permalink / raw)
  To: Guenter Roeck, Andy Shevchenko, Jakub Kicinski
  Cc: davem, axboe, viro, herbert, dong.menglong, linux-kernel, netdev

Hello,

On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> > On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net> wrote:
> >
...
>
> The problem is in net/packet/af_packet.c:packet_recvmsg(). This function,
> as well as all other rcvmsg functions, is declared as
>
> static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>                           int flags)
>
> MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
> This is then evaluated in
>
>        if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
>                 goto out;
>
> If any of those flags is declared as BIT() and thus long, flags is
> sign-extended to long. Since it is negative, its upper 32 bits will be set,
> the if statement evaluates as true, and the function bails out.
>
> This is relatively easy to fix here with, for example,
>
>         if ((unsigned int)flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
>                 goto out;
>
> but that is just a hack, and it doesn't solve the real problem:
> Each function in struct proto_ops which passes flags passes it as int
> (see include/linux/net.h:struct proto_ops). Each such function, if
> called with MSG_CMSG_COMPAT set, will fail a match against
> ~(MSG_anything) if MSG_anything is declared as BIT() or long.
>
> As it turns out, I was kind of lucky to catch the problem: So far I have
> seen it only on mips64 kernels with N32 userspace.
>
> Guenter

 Wow, now the usages of 'msg_flag' really puzzle me. Seems that
it is used as 'unsigned int' somewhere, but 'int' somewhere
else.

As I found, It is used as 'int' in 'netlink_recvmsg()',
'io_sr_msg->msg_flags', 'atalk_sendmsg()',
'dn_recvmsg()',  'proto_ops->recvmsg()', etc.

So what should I do? Revert this patch? Or fix the usages of 'flags'?
Or change the type of MSG_* to 'unsigned int'? I prefer the last
one(the usages of 'flags' can be fixed too, maybe later).


Thanks!
Menglong Dong

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  8:21       ` Menglong Dong
@ 2021-03-17  9:36         ` Andy Shevchenko
  2021-03-17  9:40           ` Andy Shevchenko
  2021-03-17 13:53           ` Menglong Dong
  2021-03-17 16:39         ` David Miller
  1 sibling, 2 replies; 15+ messages in thread
From: Andy Shevchenko @ 2021-03-17  9:36 UTC (permalink / raw)
  To: Menglong Dong
  Cc: Guenter Roeck, Jakub Kicinski, davem, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
> On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <linux@roeck-us.net> wrote:
> > On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> > > On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> ...
> >
> > The problem is in net/packet/af_packet.c:packet_recvmsg(). This function,
> > as well as all other rcvmsg functions, is declared as
> >
> > static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
> >                           int flags)
> >
> > MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
> > This is then evaluated in
> >
> >        if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
> >                 goto out;
> >
> > If any of those flags is declared as BIT() and thus long, flags is
> > sign-extended to long. Since it is negative, its upper 32 bits will be set,
> > the if statement evaluates as true, and the function bails out.
> >
> > This is relatively easy to fix here with, for example,
> >
> >         if ((unsigned int)flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
> >                 goto out;
> >
> > but that is just a hack, and it doesn't solve the real problem:
> > Each function in struct proto_ops which passes flags passes it as int
> > (see include/linux/net.h:struct proto_ops). Each such function, if
> > called with MSG_CMSG_COMPAT set, will fail a match against
> > ~(MSG_anything) if MSG_anything is declared as BIT() or long.
> >
> > As it turns out, I was kind of lucky to catch the problem: So far I have
> > seen it only on mips64 kernels with N32 userspace.
> >
> > Guenter
>
>  Wow, now the usages of 'msg_flag' really puzzle me. Seems that
> it is used as 'unsigned int' somewhere, but 'int' somewhere
> else.
>
> As I found, It is used as 'int' in 'netlink_recvmsg()',
> 'io_sr_msg->msg_flags', 'atalk_sendmsg()',
> 'dn_recvmsg()',  'proto_ops->recvmsg()', etc.
>
> So what should I do? Revert this patch? Or fix the usages of 'flags'?
> Or change the type of MSG_* to 'unsigned int'? I prefer the last
> one(the usages of 'flags' can be fixed too, maybe later).

The problematic code is negation of the flags when it's done in
operations like &.
It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
BAR) & flags.

All this is a beast called "integer promotions" in the C standard.

The best is to try to get flags to be unsigned. By how invasive it may be?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  9:36         ` Andy Shevchenko
@ 2021-03-17  9:40           ` Andy Shevchenko
  2021-03-17 10:17             ` Guenter Roeck
  2021-03-17 13:53           ` Menglong Dong
  1 sibling, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2021-03-17  9:40 UTC (permalink / raw)
  To: Menglong Dong
  Cc: Guenter Roeck, Jakub Kicinski, davem, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <menglong8.dong@gmail.com> wrote:

...

> It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
> BAR) & flags.

...and type casting will be needed anyway here...

I was thinking about this case

drivers/i2c/busses/i2c-designware-common.c:420:
dev->sda_hold_time & ~(u32)DW_IC_SDA_HOLD_RX_MASK
,

but sda_hold_time there is unsigned.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  9:40           ` Andy Shevchenko
@ 2021-03-17 10:17             ` Guenter Roeck
  0 siblings, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-03-17 10:17 UTC (permalink / raw)
  To: Andy Shevchenko, Menglong Dong
  Cc: Jakub Kicinski, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

On 3/17/21 2:40 AM, Andy Shevchenko wrote:
> On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <menglong8.dong@gmail.com> wrote:
> 
> ...
> 
>> It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
>> BAR) & flags.
> 
> ...and type casting will be needed anyway here...
> 
> I was thinking about this case
> 
> drivers/i2c/busses/i2c-designware-common.c:420:
> dev->sda_hold_time & ~(u32)DW_IC_SDA_HOLD_RX_MASK
> ,
> > but sda_hold_time there is unsigned.
> 
That is needed because of the %d. Without the (u32), the expression is
promoted to unsigned long and the compiler wants to see %ld.

Guenter


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  9:36         ` Andy Shevchenko
  2021-03-17  9:40           ` Andy Shevchenko
@ 2021-03-17 13:53           ` Menglong Dong
  2021-03-17 14:15             ` Menglong Dong
  2021-03-17 15:02             ` Guenter Roeck
  1 sibling, 2 replies; 15+ messages in thread
From: Menglong Dong @ 2021-03-17 13:53 UTC (permalink / raw)
  To: Andy Shevchenko, Guenter Roeck
  Cc: Jakub Kicinski, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

On Wed, Mar 17, 2021 at 5:36 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
...
>
> The problematic code is negation of the flags when it's done in
> operations like &.
> It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
> BAR) & flags.
>
> All this is a beast called "integer promotions" in the C standard.
>
> The best is to try to get flags to be unsigned. By how invasive it may be?

Seems that the inconsistent usages of 'msg_flags' is a lot, for example the
'recvmsg()' in 'struct proto' and 'recvmsg()' in 'struct proto_ops':

int (*recvmsg)(struct sock *sk, struct msghdr *msg,
        size_t len, int noblock, int flags,
        int *addr_len);

This function prototype is used in many places, It's not easy to fix them.
This patch is already reverted, and I think maybe
I can resend it after I fix these 'int' flags.

>
> --
> With Best Regards,
> Andy Shevchenko

Thanks!
Menglong Dong

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17 13:53           ` Menglong Dong
@ 2021-03-17 14:15             ` Menglong Dong
  2021-03-17 15:02             ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: Menglong Dong @ 2021-03-17 14:15 UTC (permalink / raw)
  To: Andy Shevchenko, Jakub Kicinski
  Cc: Guenter Roeck, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

On Wed, Mar 17, 2021 at 9:53 PM Menglong Dong <menglong8.dong@gmail.com> wrote:
>
...
>
> Seems that the inconsistent usages of 'msg_flags' is a lot, for example the
> 'recvmsg()' in 'struct proto' and 'recvmsg()' in 'struct proto_ops':
>
> int (*recvmsg)(struct sock *sk, struct msghdr *msg,
>         size_t len, int noblock, int flags,
>         int *addr_len);
>
> This function prototype is used in many places, It's not easy to fix them.
> This patch is already reverted, and I think maybe
> I can resend it after I fix these 'int' flags.
>

I doubt it now...there are hundreds of functions that are defined as
'proto_ops->recvmsg()'.
enn...will this kind of patch be acceptable? Is it the time to give up?

With Best Regards,
Menglong Dong

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17 13:53           ` Menglong Dong
  2021-03-17 14:15             ` Menglong Dong
@ 2021-03-17 15:02             ` Guenter Roeck
  1 sibling, 0 replies; 15+ messages in thread
From: Guenter Roeck @ 2021-03-17 15:02 UTC (permalink / raw)
  To: Menglong Dong
  Cc: Andy Shevchenko, Jakub Kicinski, davem, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

On Wed, Mar 17, 2021 at 09:53:23PM +0800, Menglong Dong wrote:
> On Wed, Mar 17, 2021 at 5:36 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> ...
> >
> > The problematic code is negation of the flags when it's done in
> > operations like &.
> > It maybe fixed by swapping positions of the arguments, i.e. ~(FOO |
> > BAR) & flags.
> >
> > All this is a beast called "integer promotions" in the C standard.
> >
> > The best is to try to get flags to be unsigned. By how invasive it may be?
> 
> Seems that the inconsistent usages of 'msg_flags' is a lot, for example the
> 'recvmsg()' in 'struct proto' and 'recvmsg()' in 'struct proto_ops':
> 
> int (*recvmsg)(struct sock *sk, struct msghdr *msg,
>         size_t len, int noblock, int flags,
>         int *addr_len);
> 
> This function prototype is used in many places, It's not easy to fix them.

Also, flags is used in several other functions, not just recvmsg.

> This patch is already reverted, and I think maybe
> I can resend it after I fix these 'int' flags.

I would suggest to consult with Dave on that. While much of the conversion
could be handled automatically with coccinelle, it touches a lot of files.
I don't think that is worth the effort (or risk).

Guenter

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  1:37     ` Guenter Roeck
  2021-03-17  8:21       ` Menglong Dong
@ 2021-03-17 15:12       ` David Laight
  2021-03-18  1:48         ` Menglong Dong
  1 sibling, 1 reply; 15+ messages in thread
From: David Laight @ 2021-03-17 15:12 UTC (permalink / raw)
  To: 'Guenter Roeck', Andy Shevchenko
  Cc: menglong8.dong, kuba, davem, axboe, viro, herbert, dong.menglong,
	linux-kernel, netdev

From: Guenter Roeck
> Sent: 17 March 2021 01:38
...
> MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
> This is then evaluated in
> 
>        if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
>                 goto out;
> 
> If any of those flags is declared as BIT() and thus long, flags is
> sign-extended to long. Since it is negative, its upper 32 bits will be set,
> the if statement evaluates as true, and the function bails out.
> 
> This is relatively easy to fix here with, for example,
> 
>         if ((unsigned int)flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
>                 goto out;
> 
> but that is just a hack, and it doesn't solve the real problem:
> Each function in struct proto_ops which passes flags passes it as int
> (see include/linux/net.h:struct proto_ops). Each such function, if
> called with MSG_CMSG_COMPAT set, will fail a match against
> ~(MSG_anything) if MSG_anything is declared as BIT() or long.

Isn't MSG_CMSG_COMPAT an internal value?
Could it be changed to 1u << 30 instead of 1u << 31 ?
Then it wouldn't matter if the high bit of flags got replicated.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17  8:21       ` Menglong Dong
  2021-03-17  9:36         ` Andy Shevchenko
@ 2021-03-17 16:39         ` David Miller
  1 sibling, 0 replies; 15+ messages in thread
From: David Miller @ 2021-03-17 16:39 UTC (permalink / raw)
  To: menglong8.dong
  Cc: linux, andy.shevchenko, kuba, axboe, viro, herbert,
	dong.menglong, linux-kernel, netdev

From: Menglong Dong <menglong8.dong@gmail.com>
Date: Wed, 17 Mar 2021 16:21:14 +0800

> Hello,
> 
> On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
>> > On Wednesday, March 17, 2021, Guenter Roeck <linux@roeck-us.net> wrote:
>> >
> ...
>>
>> The problem is in net/packet/af_packet.c:packet_recvmsg(). This function,
>> as well as all other rcvmsg functions, is declared as
>>
>> static int packet_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>>                           int flags)
>>
>> MSG_CMSG_COMPAT (0x80000000) is set in flags, meaning its value is negative.
>> This is then evaluated in
>>
>>        if (flags & ~(MSG_PEEK|MSG_DONTWAIT|MSG_TRUNC|MSG_CMSG_COMPAT|MSG_ERRQUEUE))
>>                 goto out;
> So what should I do? Revert this patch? Or fix the usages of 'flags'?

I already reverted this patch from net-next to fix the regression.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_*
  2021-03-17 15:12       ` David Laight
@ 2021-03-18  1:48         ` Menglong Dong
  0 siblings, 0 replies; 15+ messages in thread
From: Menglong Dong @ 2021-03-18  1:48 UTC (permalink / raw)
  To: David Laight
  Cc: Guenter Roeck, Andy Shevchenko, kuba, davem, axboe, viro,
	herbert, dong.menglong, linux-kernel, netdev

On Wed, Mar 17, 2021 at 11:12 PM David Laight <David.Laight@aculab.com> wrote:
>
...
>
> Isn't MSG_CMSG_COMPAT an internal value?
> Could it be changed to 1u << 30 instead of 1u << 31 ?
> Then it wouldn't matter if the high bit of flags got replicated.
>

Yeah, MSG_CMSG_COMPAT is an internal value, and maybe
it's why it is defined as 1<< 31, to make it look different.

I think it's a good idea to change it to other value which is
not used, such as 1u<<21.

I will test it and resend this patch later, thanks~

With Regards,
Menglong Dong

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2021-03-18  1:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-10  1:51 [PATCH v4 RESEND net-next] net: socket: use BIT() for MSG_* menglong8.dong
2021-03-10 21:00 ` patchwork-bot+netdevbpf
2021-03-16 22:48 ` Guenter Roeck
     [not found]   ` <CAHp75VdE3fkCjb53vBso5uJX9aEFtAOAdh5NVOSbK0YR64+jOg@mail.gmail.com>
2021-03-16 23:54     ` Guenter Roeck
2021-03-17  1:37     ` Guenter Roeck
2021-03-17  8:21       ` Menglong Dong
2021-03-17  9:36         ` Andy Shevchenko
2021-03-17  9:40           ` Andy Shevchenko
2021-03-17 10:17             ` Guenter Roeck
2021-03-17 13:53           ` Menglong Dong
2021-03-17 14:15             ` Menglong Dong
2021-03-17 15:02             ` Guenter Roeck
2021-03-17 16:39         ` David Miller
2021-03-17 15:12       ` David Laight
2021-03-18  1:48         ` Menglong Dong

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).