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

Michael