2019-06-27 03:26:07

by Jiunn Chang

[permalink] [raw]
Subject: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

Shifting signed 32-bit value by 31 bits is undefined. Changing most
significant bit to unsigned.

Changes included in v2:
- use subsystem specific subject lines
- CC required mailing lists

Signed-off-by: Jiunn Chang <[email protected]>
---
include/uapi/linux/if_packet.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index 467b654bd4c7..3d884d68eb30 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -123,7 +123,7 @@ struct tpacket_auxdata {
/* Rx and Tx ring - header status */
#define TP_STATUS_TS_SOFTWARE (1 << 29)
#define TP_STATUS_TS_SYS_HARDWARE (1 << 30) /* deprecated, never set */
-#define TP_STATUS_TS_RAW_HARDWARE (1 << 31)
+#define TP_STATUS_TS_RAW_HARDWARE (1U << 31)

/* Rx ring - feature request bits */
#define TP_FT_REQ_FILL_RXHASH 0x1
--
2.22.0


2019-06-27 03:35:14

by Shuah Khan

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

On 6/26/19 9:25 PM, Jiunn Chang wrote:
> Shifting signed 32-bit value by 31 bits is undefined. Changing most
> significant bit to unsigned.
>
> Changes included in v2:
> - use subsystem specific subject lines
> - CC required mailing lists
>

These version change lines don't belong in the change log.

> Signed-off-by: Jiunn Chang <[email protected]>
> ---

Move them here.

> include/uapi/linux/if_packet.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
> index 467b654bd4c7..3d884d68eb30 100644
> --- a/include/uapi/linux/if_packet.h
> +++ b/include/uapi/linux/if_packet.h
> @@ -123,7 +123,7 @@ struct tpacket_auxdata {
> /* Rx and Tx ring - header status */
> #define TP_STATUS_TS_SOFTWARE (1 << 29)
> #define TP_STATUS_TS_SYS_HARDWARE (1 << 30) /* deprecated, never set */
> -#define TP_STATUS_TS_RAW_HARDWARE (1 << 31)
> +#define TP_STATUS_TS_RAW_HARDWARE (1U << 31)
>
> /* Rx ring - feature request bits */
> #define TP_FT_REQ_FILL_RXHASH 0x1
>

thanks,
-- Shuah

2019-06-27 05:05:12

by Jiunn Chang

[permalink] [raw]
Subject: [Linux-kernel-mentees][PATCH v3] packet: Fix undefined behavior in bit shift

Shifting signed 32-bit value by 31 bits is undefined. Changing most
significant bit to unsigned.

Signed-off-by: Jiunn Chang <[email protected]>
---
Changes included in v3:
- remove change log from patch description

Changes included in v2:
- use subsystem specific subject lines
- CC required mailing lists

include/uapi/linux/if_packet.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/uapi/linux/if_packet.h b/include/uapi/linux/if_packet.h
index 467b654bd4c7..3d884d68eb30 100644
--- a/include/uapi/linux/if_packet.h
+++ b/include/uapi/linux/if_packet.h
@@ -123,7 +123,7 @@ struct tpacket_auxdata {
/* Rx and Tx ring - header status */
#define TP_STATUS_TS_SOFTWARE (1 << 29)
#define TP_STATUS_TS_SYS_HARDWARE (1 << 30) /* deprecated, never set */
-#define TP_STATUS_TS_RAW_HARDWARE (1 << 31)
+#define TP_STATUS_TS_RAW_HARDWARE (1U << 31)

/* Rx ring - feature request bits */
#define TP_FT_REQ_FILL_RXHASH 0x1
--
2.22.0

2019-06-27 16:23:41

by David Miller

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

From: Shuah Khan <[email protected]>
Date: Wed, 26 Jun 2019 21:32:52 -0600

> On 6/26/19 9:25 PM, Jiunn Chang wrote:
>> Shifting signed 32-bit value by 31 bits is undefined. Changing most
>> significant bit to unsigned.
>> Changes included in v2:
>> - use subsystem specific subject lines
>> - CC required mailing lists
>>
>
> These version change lines don't belong in the change log.

For networking changes I actually like the change lines to be in the
commit log. So please don't stray people this way, thanks.

2019-06-27 16:53:02

by Shuah Khan

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

On 6/27/19 10:22 AM, David Miller wrote:
> From: Shuah Khan <[email protected]>
> Date: Wed, 26 Jun 2019 21:32:52 -0600
>
>> On 6/26/19 9:25 PM, Jiunn Chang wrote:
>>> Shifting signed 32-bit value by 31 bits is undefined. Changing most
>>> significant bit to unsigned.
>>> Changes included in v2:
>>> - use subsystem specific subject lines
>>> - CC required mailing lists
>>>
>>
>> These version change lines don't belong in the change log.
>
> For networking changes I actually like the change lines to be in the
> commit log. So please don't stray people this way, thanks.
>

As a general rule, please don't include change lines in the commit log.
For networking changes that get sent to David and netdev, as David
points out here, he likes them in the commit log, please include them
in the commit log.

I am working on FAQ (Frequently Answered Questions) section for mentees.
I will add this to it.

thanks,
-- Shuah

2019-06-27 16:59:33

by Jiunn Chang

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

On Thu, Jun 27, 2019 at 09:22:53AM -0700, David Miller wrote:
> From: Shuah Khan <[email protected]>
> Date: Wed, 26 Jun 2019 21:32:52 -0600
>
> > On 6/26/19 9:25 PM, Jiunn Chang wrote:
> >> Shifting signed 32-bit value by 31 bits is undefined. Changing most
> >> significant bit to unsigned.
> >> Changes included in v2:
> >> - use subsystem specific subject lines
> >> - CC required mailing lists
> >>
> >
> > These version change lines don't belong in the change log.
>
> For networking changes I actually like the change lines to be in the
> commit log. So please don't stray people this way, thanks.

Hello David,

Would you like me to send v3 with the change log in the patch description?

I would be happy to do that.

THX,

Jiunn

2019-06-27 17:06:36

by Alexei Starovoitov

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

On Thu, Jun 27, 2019 at 9:54 AM Shuah Khan <[email protected]> wrote:
>
> On 6/27/19 10:22 AM, David Miller wrote:
> > From: Shuah Khan <[email protected]>
> > Date: Wed, 26 Jun 2019 21:32:52 -0600
> >
> >> On 6/26/19 9:25 PM, Jiunn Chang wrote:
> >>> Shifting signed 32-bit value by 31 bits is undefined. Changing most
> >>> significant bit to unsigned.
> >>> Changes included in v2:
> >>> - use subsystem specific subject lines
> >>> - CC required mailing lists
> >>>
> >>
> >> These version change lines don't belong in the change log.
> >
> > For networking changes I actually like the change lines to be in the
> > commit log. So please don't stray people this way, thanks.
> >
>
> As a general rule, please don't include change lines in the commit log.
> For networking changes that get sent to David and netdev, as David
> points out here, he likes them in the commit log, please include them
> in the commit log.
>
> I am working on FAQ (Frequently Answered Questions) section for mentees.
> I will add this to it.

Same for bpf trees.
We prefer developers put as much as info as possible into commit logs
and cover letters.
Explanation of v1->v2->v3 differences is invaluable not only at
the point of code review, but in the future.

2019-06-27 17:10:43

by Shuah Khan

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

On 6/27/19 11:05 AM, Alexei Starovoitov wrote:
> On Thu, Jun 27, 2019 at 9:54 AM Shuah Khan <[email protected]> wrote:
>>
>> On 6/27/19 10:22 AM, David Miller wrote:
>>> From: Shuah Khan <[email protected]>
>>> Date: Wed, 26 Jun 2019 21:32:52 -0600
>>>
>>>> On 6/26/19 9:25 PM, Jiunn Chang wrote:
>>>>> Shifting signed 32-bit value by 31 bits is undefined. Changing most
>>>>> significant bit to unsigned.
>>>>> Changes included in v2:
>>>>> - use subsystem specific subject lines
>>>>> - CC required mailing lists
>>>>>
>>>>
>>>> These version change lines don't belong in the change log.
>>>
>>> For networking changes I actually like the change lines to be in the
>>> commit log. So please don't stray people this way, thanks.
>>>
>>
>> As a general rule, please don't include change lines in the commit log.
>> For networking changes that get sent to David and netdev, as David
>> points out here, he likes them in the commit log, please include them
>> in the commit log.
>>
>> I am working on FAQ (Frequently Answered Questions) section for mentees.
>> I will add this to it.
>
> Same for bpf trees.
> We prefer developers put as much as info as possible into commit logs
> and cover letters.
> Explanation of v1->v2->v3 differences is invaluable not only at
> the point of code review, but in the future.
>

Thanks Alex. I will add that to the FAQ.

-- Shuah

2019-06-27 17:35:25

by David Miller

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

From: Jiunn Chang <[email protected]>
Date: Thu, 27 Jun 2019 11:57:28 -0500

> On Thu, Jun 27, 2019 at 09:22:53AM -0700, David Miller wrote:
>> From: Shuah Khan <[email protected]>
>> Date: Wed, 26 Jun 2019 21:32:52 -0600
>>
>> > On 6/26/19 9:25 PM, Jiunn Chang wrote:
>> >> Shifting signed 32-bit value by 31 bits is undefined. Changing most
>> >> significant bit to unsigned.
>> >> Changes included in v2:
>> >> - use subsystem specific subject lines
>> >> - CC required mailing lists
>> >>
>> >
>> > These version change lines don't belong in the change log.
>>
>> For networking changes I actually like the change lines to be in the
>> commit log. So please don't stray people this way, thanks.
>
> Hello David,
>
> Would you like me to send v3 with the change log in the patch description?

I'll use v2 which had this done correctly.

2019-06-29 18:07:14

by David Miller

[permalink] [raw]
Subject: Re: [Linux-kernel-mentees][PATCH v2] packet: Fix undefined behavior in bit shift

From: Jiunn Chang <[email protected]>
Date: Wed, 26 Jun 2019 22:25:30 -0500

> Shifting signed 32-bit value by 31 bits is undefined. Changing most
> significant bit to unsigned.
>
> Changes included in v2:
> - use subsystem specific subject lines
> - CC required mailing lists
>
> Signed-off-by: Jiunn Chang <[email protected]>

Applied.