> -----Original Message----- > From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx> > Sent: Tuesday, August 15, 2023 1:46 AM > To: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>; Saurabh Singh Sengar > <ssengar@xxxxxxxxxxxxx>; Saurabh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx>; > KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang > <haiyangz@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Dexuan Cui > <decui@xxxxxxxxxxxxx> > Cc: linux-hyperv@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: RE: [PATCH] hv: hyperv.h: Replace one-element array with flexible- > array member > > From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx> Sent: Monday, > August 14, 2023 1:10 PM > > From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxx> Sent: Tuesday, > > August 8, 2023 2:49 AM > > > > > [snip] > > > > > > > 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? + 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
