From: Menglong Dong <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
>
> 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 <[email protected]>
>
> [...]
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
Hi,
On Tue, Mar 09, 2021 at 05:51:35PM -0800, [email protected] wrote:
> From: Menglong Dong <[email protected]>
>
> 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 <[email protected]>
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_*
On 3/16/21 4:02 PM, Andy Shevchenko wrote:
>
>
> On Wednesday, March 17, 2021, Guenter Roeck <[email protected] <mailto:[email protected]>> wrote:
>
> Hi,
>
> On Tue, Mar 09, 2021 at 05:51:35PM -0800, [email protected] <mailto:[email protected]> wrote:
> > From: Menglong Dong <[email protected] <mailto:[email protected]>>
> >
> > 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 <[email protected] <mailto:[email protected]>>
>
> 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
>
>
On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> On Wednesday, March 17, 2021, Guenter Roeck <[email protected]> wrote:
>
> > Hi,
> >
> > On Tue, Mar 09, 2021 at 05:51:35PM -0800, [email protected] wrote:
> > > From: Menglong Dong <[email protected]>
> > >
> > > 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 <[email protected]>
> >
> > 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
Hello,
On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <[email protected]> wrote:
>
> On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> > On Wednesday, March 17, 2021, Guenter Roeck <[email protected]> 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
On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <[email protected]> wrote:
> On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <[email protected]> wrote:
> > On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
> > > On Wednesday, March 17, 2021, Guenter Roeck <[email protected]> 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
On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
<[email protected]> wrote:
> On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <[email protected]> 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
On 3/17/21 2:40 AM, Andy Shevchenko wrote:
> On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
> <[email protected]> wrote:
>> On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong <[email protected]> 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
On Wed, Mar 17, 2021 at 5:36 PM Andy Shevchenko
<[email protected]> 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
From: Menglong Dong <[email protected]>
Date: Wed, 17 Mar 2021 16:21:14 +0800
> Hello,
>
> On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck <[email protected]> wrote:
>>
>> On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
>> > On Wednesday, March 17, 2021, Guenter Roeck <[email protected]> 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.
On Wed, Mar 17, 2021 at 9:53 PM Menglong Dong <[email protected]> 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
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)
On Wed, Mar 17, 2021 at 09:53:23PM +0800, Menglong Dong wrote:
> On Wed, Mar 17, 2021 at 5:36 PM Andy Shevchenko
> <[email protected]> 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
On Wed, Mar 17, 2021 at 11:12 PM David Laight <[email protected]> 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