As you noted, switching to a flexible array member changes the
size of struct vmtransfer_page_packet_header. In netvsc_receive()
the size of this structure is used in validation of the VMbus packet
received from Hyper-V. Changing the size of the structure changes
the validation. The validation code probably needs to be adjusted
to account for the new structure size (or the original validation code
was wrong).
There might be other places in the code that are affected by a change
in the size of this structure. I haven't fully investigated.
Michael
-----Original Message-----
From: Michael Kelley (LINUX) <redacted>
Sent: Monday, August 7, 2023 10:26 PM
To: Saurabh Sengar <ssengar@linux.microsoft.com>; KY Srinivasan
[off-list ref]; Haiyang Zhang [off-list ref];
wei.liu@kernel.org; Dexuan Cui [off-list ref]
Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Saurabh
Singh Sengar [off-list ref]
Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
array member
From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Monday, August
7, 2023 2:51 AM
quoted
One-element and zero-length arrays are deprecated. Replace one-element
array in struct vmtransfer_page_packet_header with flexible-array
member. This change fixes below warning:
[ 2.593788]
As you noted, switching to a flexible array member changes the
size of struct vmtransfer_page_packet_header. In netvsc_receive()
the size of this structure is used in validation of the VMbus packet received
from Hyper-V. Changing the size of the structure changes
the validation. The validation code probably needs to be adjusted
to account for the new structure size (or the original validation code was
wrong).
There might be other places in the code that are affected by a change in the
size of this structure. I haven't fully investigated.
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make this change.
Through my analysis, I arrived at the conclusion that the initial validation code
wasn't entirely accurate. And with the proposed changes it gets more accurate.
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size validation"
is only to make sure that header is correct. So, that we fetch the range_cnt and
xfer_pageset_id correctly from it. For this to be done I don't find any reason
to include the size of ranges in this check. With inclusion of ranges we are
checking the first 'struct vmtransfer_page_range' size as well which is not required
for fetching above two values.
Once we have the count of ranges we will anyway check the sanity of ranges with
NETVSC_XFER_HEADER_SIZE. This will check "count * (struct vmtransfer_page_range)"
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between both the checks as
of today.
Please let me know you opinion if you don't find my explanation reasonable.
I don't see any other place this structure's size change will affect.
- Saurabh
From: Michael Kelley (LINUX) <hidden> Date: 2023-08-14 20:10:54
From: Saurabh Singh Sengar <redacted> Sent: Tuesday, August 8, 2023 2:49 AM
quoted
-----Original Message-----
From: Michael Kelley (LINUX) <redacted>
Sent: Monday, August 7, 2023 10:26 PM
To: Saurabh Sengar <ssengar@linux.microsoft.com>; KY Srinivasan
[off-list ref]; Haiyang Zhang [off-list ref];
wei.liu@kernel.org; Dexuan Cui [off-list ref]
Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org; Saurabh
Singh Sengar [off-list ref]
Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
array member
From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Monday, August 7, 2023 2:51 AM
quoted
One-element and zero-length arrays are deprecated. Replace one-element
array in struct vmtransfer_page_packet_header with flexible-array
member. This change fixes below warning:
[ 2.593788]
As you noted, switching to a flexible array member changes the
size of struct vmtransfer_page_packet_header. In netvsc_receive()
the size of this structure is used in validation of the VMbus packet received
from Hyper-V. Changing the size of the structure changes
the validation. The validation code probably needs to be adjusted
to account for the new structure size (or the original validation code was
wrong).
There might be other places in the code that are affected by a change in the
size of this structure. I haven't fully investigated.
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make this change.
Through my analysis, I arrived at the conclusion that the initial validation code
wasn't entirely accurate. And with the proposed changes it gets more accurate.
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size validation"
is only to make sure that header is correct. So, that we fetch the range_cnt and
xfer_pageset_id correctly from it. For this to be done I don't find any reason
to include the size of ranges in this check. With inclusion of ranges we are
checking the first 'struct vmtransfer_page_range' size as well which is not required
for fetching above two values.
Once we have the count of ranges we will anyway check the sanity of ranges with
NETVSC_XFER_HEADER_SIZE. This will check "count * (struct vmtransfer_page_range)"
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between both the checks as
of today.
Please let me know you opinion if you don't find my explanation reasonable.
I don't see any other place this structure's size change will affect.
Got it. I have now carefully looked at the netvsc_receive() code myself,
and I agree with you. With the 1 element array, the validation in
netvsc_receive() could have generated a false positive if the range_cnt
is zero. But I don't think a zero range_cnt ever happens, so the
false positive never happens. With the change to use a flexible array,
the validation is now correct even with a range_cnt of zero.
Please add a note to the commit message for this patch: The validation
code in the netvsc driver is affected by changing the struct size, but the
effects have been examined and have been determined to be appropriate.
Michael
From: Michael Kelley (LINUX) <hidden> Date: 2023-08-14 20:16:30
From: Michael Kelley (LINUX) <redacted> Sent: Monday, August 14, 2023 1:10 PM
From: Saurabh Singh Sengar <redacted> Sent: Tuesday, August 8, 2023 2:49 AM
quoted
[snip]
quoted
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make this change.
Through my analysis, I arrived at the conclusion that the initial validation code
wasn't entirely accurate. And with the proposed changes it gets more accurate.
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size validation"
is only to make sure that header is correct. So, that we fetch the range_cnt and
xfer_pageset_id correctly from it. For this to be done I don't find any reason
to include the size of ranges in this check. With inclusion of ranges we are
checking the first 'struct vmtransfer_page_range' size as well which is not required
for fetching above two values.
Once we have the count of ranges we will anyway check the sanity of ranges with
NETVSC_XFER_HEADER_SIZE. This will check "count * (struct vmtransfer_page_range)"
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between both the checks as
of today.
Please let me know you opinion if you don't find my explanation reasonable.
I don't see any other place this structure's size change will affect.
Got it. I have now carefully looked at the netvsc_receive() code myself,
and I agree with you. With the 1 element array, the validation in
netvsc_receive() could have generated a false positive if the range_cnt
is zero. But I don't think a zero range_cnt ever happens, so the
false positive never happens. With the change to use a flexible array,
the validation is now correct even with a range_cnt of zero.
Please add a note to the commit message for this patch: The validation
code in the netvsc driver is affected by changing the struct size, but the
effects have been examined and have been determined to be appropriate.
One other thought: Could this change affect user space DPDK code that
is processing netvsc packets?
Michael
-----Original Message-----
From: Michael Kelley (LINUX) <redacted>
Sent: Tuesday, August 15, 2023 1:46 AM
To: Michael Kelley (LINUX) <redacted>; Saurabh Singh Sengar
[off-list ref]; Saurabh Sengar [off-list ref];
KY Srinivasan [off-list ref]; Haiyang Zhang
[off-list ref]; wei.liu@kernel.org; Dexuan Cui
[off-list ref]
Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
array member
From: Michael Kelley (LINUX) <redacted> Sent: Monday,
August 14, 2023 1:10 PM
quoted
From: Saurabh Singh Sengar <redacted> Sent: Tuesday,
August 8, 2023 2:49 AM
quoted
[snip]
quoted
quoted
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make
this change.
quoted
quoted
Through my analysis, I arrived at the conclusion that the initial
validation code wasn't entirely accurate. And with the proposed changes it
gets more accurate.
quoted
quoted
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size
validation"
quoted
quoted
is only to make sure that header is correct. So, that we fetch the
range_cnt and xfer_pageset_id correctly from it. For this to be done
I don't find any reason to include the size of ranges in this check.
With inclusion of ranges we are checking the first 'struct
vmtransfer_page_range' size as well which is not required for fetching
above two values.
quoted
quoted
Once we have the count of ranges we will anyway check the sanity of
ranges with NETVSC_XFER_HEADER_SIZE. This will check "count * (struct
vmtransfer_page_range)"
quoted
quoted
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between
both the checks as of today.
Please let me know you opinion if you don't find my explanation
reasonable.
quoted
quoted
I don't see any other place this structure's size change will affect.
Got it. I have now carefully looked at the netvsc_receive() code
myself, and I agree with you. With the 1 element array, the
validation in
netvsc_receive() could have generated a false positive if the
range_cnt is zero. But I don't think a zero range_cnt ever happens,
so the false positive never happens. With the change to use a
flexible array, the validation is now correct even with a range_cnt of zero.
Please add a note to the commit message for this patch: The
validation code in the netvsc driver is affected by changing the
struct size, but the effects have been examined and have been determined
to be appropriate.
quoted
One other thought: Could this change affect user space DPDK code that is
processing netvsc packets?
+ Long Li
I am aware that DPDK code uses uio_hv_generic driver to have its own
implementation of userspace netvsc and the changes here are only confined
to kernel's netvsc driver. Thus, I believe this code shouldn't affect anything
in userspace netvsc implementation.
I also browsed the DPDK code and found that DPDK has this struct implemented as
struct vmbus_chanpkt_rxbuf and that already has flexible array member.
https://github.com/DPDK/dpdk/blob/v23.07/drivers/bus/vmbus/rte_vmbus_reg.h#L182
- Saurabh
From: Michael Kelley (LINUX) <hidden> Date: 2023-08-15 16:13:26
From: Saurabh Singh Sengar <redacted> Sent: Tuesday, August 15, 2023 7:59 AM
quoted
-----Original Message-----
From: Michael Kelley (LINUX) <redacted>
Sent: Tuesday, August 15, 2023 1:46 AM
To: Michael Kelley (LINUX) <redacted>; Saurabh Singh Sengar
[off-list ref]; Saurabh Sengar [off-list ref];
KY Srinivasan [off-list ref]; Haiyang Zhang
[off-list ref]; wei.liu@kernel.org; Dexuan Cui
[off-list ref]
Cc: linux-hyperv@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-
array member
From: Michael Kelley (LINUX) <redacted> Sent: Monday,
August 14, 2023 1:10 PM
quoted
From: Saurabh Singh Sengar <redacted> Sent: Tuesday,
August 8, 2023 2:49 AM
quoted
[snip]
quoted
quoted
Thanks for your comment, I wanted to have this discussion.
Before sending this patch, I was contemplating whether or not to make this change.
Through my analysis, I arrived at the conclusion that the initial
validation code wasn't entirely accurate. And with the proposed changes it gets more accurate.
IMHO it is more accurate to exclude the size of 'ranges' in the header.
With my limited understanding of this driver, the current "header size validation"
is only to make sure that header is correct. So, that we fetch the
range_cnt and xfer_pageset_id correctly from it. For this to be done
I don't find any reason to include the size of ranges in this check.
With inclusion of ranges we are checking the first 'struct
vmtransfer_page_range' size as well which is not required for fetching above two values.
Once we have the count of ranges we will anyway check the sanity of
ranges with NETVSC_XFER_HEADER_SIZE. This will check "count * (struct vmtransfer_page_range)"
Which is present few lines after.
For a ranges count = 1, I don't see there is any difference between
both the checks as of today.
Please let me know you opinion if you don't find my explanation reasonable.
I don't see any other place this structure's size change will affect.
Got it. I have now carefully looked at the netvsc_receive() code
myself, and I agree with you. With the 1 element array, the validation in
netvsc_receive() could have generated a false positive if the
range_cnt is zero. But I don't think a zero range_cnt ever happens,
so the false positive never happens. With the change to use a
flexible array, the validation is now correct even with a range_cnt of zero.
Please add a note to the commit message for this patch: The
validation code in the netvsc driver is affected by changing the
struct size, but the effects have been examined and have been determined
to be appropriate.
quoted
One other thought: Could this change affect user space DPDK code that is
processing netvsc packets?
+ Long Li
I am aware that DPDK code uses uio_hv_generic driver to have its own
implementation of userspace netvsc and the changes here are only confined
to kernel's netvsc driver. Thus, I believe this code shouldn't affect anything
in userspace netvsc implementation.
I also browsed the DPDK code and found that DPDK has this struct implemented as
struct vmbus_chanpkt_rxbuf and that already has flexible array member.
https://github.com/DPDK/dpdk/blob/v23.07/drivers/bus/vmbus/rte_vmbus_reg.h#L182