On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
On Wednesday, March 17, 2021, Guenter Roeck [off-list ref] wrote:
quoted
Hi,
On Tue, Mar 09, 2021 at 05:51:35PM -0800, menglong8.dong@gmail.com wrote:
quoted
From: Menglong Dong <redacted>
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 <redacted>
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 [off-list ref] wrote:
On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
quoted
On Wednesday, March 17, 2021, Guenter Roeck [off-list ref] 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
From: Andy Shevchenko <hidden> Date: 2021-03-17 09:37:04
On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong [off-list ref] wrote:
On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
quoted
On Wednesday, March 17, 2021, Guenter Roeck [off-list ref] wrote:
...
quoted
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
From: Andy Shevchenko <hidden> Date: 2021-03-17 09:41:20
On Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
[off-list ref] wrote:
On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong [off-list ref] 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 Wed, Mar 17, 2021 at 11:36 AM Andy Shevchenko
[off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 10:21 AM Menglong Dong [off-list ref] wrote:
...
quoted
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
,
quoted
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
[off-list ref] 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.
On Wed, Mar 17, 2021 at 9:53 PM Menglong Dong [off-list ref] 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: David Laight <hidden> Date: 2021-03-17 15:59:58
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
[off-list ref] wrote:
quoted
...
quoted
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
Hello,
On Wed, Mar 17, 2021 at 9:38 AM Guenter Roeck [off-list ref] wrote:
quoted
On Wed, Mar 17, 2021 at 01:02:51AM +0200, Andy Shevchenko wrote:
quoted
On Wednesday, March 17, 2021, Guenter Roeck [off-list ref] wrote:
...
quoted
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 11:12 PM David Laight [off-list ref] 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