lkml.org 
[lkml]   [2023]   [Aug]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [PATCH] hv: hyperv.h: Replace one-element array with flexible-array member
    Date
    From: Saurabh Singh Sengar <ssengar@microsoft.com> Sent: Tuesday, August 15, 2023 7:59 AM
    >
    > > -----Original Message-----
    > > From: Michael Kelley (LINUX) <mikelley@microsoft.com>
    > > Sent: Tuesday, August 15, 2023 1:46 AM
    > > To: Michael Kelley (LINUX) <mikelley@microsoft.com>; Saurabh Singh Sengar
    > > <ssengar@microsoft.com>; Saurabh Sengar <ssengar@linux.microsoft.com>;
    > > KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
    > > <haiyangz@microsoft.com>; wei.liu@kernel.org; Dexuan Cui
    > > <decui@microsoft.com>
    > > 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) <mikelley@microsoft.com> Sent: Monday,
    > > August 14, 2023 1:10 PM
    > > > From: Saurabh Singh Sengar <ssengar@microsoft.com> 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
    >

    Sounds good to me. Thanks for checking.

    Michael

    \
     
     \ /
      Last update: 2023-08-15 18:14    [W:4.546 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site