netdev - RE: [PATCH v3] net: hyperv: add support for vlans in netvsc driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <BL0PR2101MB0930B942CE2E4EB875EC38C2CA780@BL0PR2101MB0930.namprd21.prod.outlook.com>
Date:   Tue, 21 Jul 2020 22:18:19 +0000
From:   Haiyang Zhang <haiyangz@...rosoft.com>
To:     "Sriram Krishnan (srirakr2)" <srirakr2@...co.com>,
        David Miller <davem@...emloft.net>
CC:     KY Srinivasan <kys@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        "wei.liu@...nel.org" <wei.liu@...nel.org>,
        "Malcolm Bumgardner (mbumgard)" <mbumgard@...co.com>,
        "Umesha G M (ugm)" <ugm@...co.com>,
        "Niranjan M M (nimm)" <nimm@...co.com>,
        "xe-linux-external(mailer list)" <xe-linux-external@...co.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v3] net: hyperv: add support for vlans in netvsc driver



> -----Original Message-----
> From: Sriram Krishnan (srirakr2) <srirakr2@...co.com>
> Sent: Tuesday, July 21, 2020 3:10 AM
> To: David Miller <davem@...emloft.net>
> Cc: KY Srinivasan <kys@...rosoft.com>; Haiyang Zhang
> <haiyangz@...rosoft.com>; Stephen Hemminger <sthemmin@...rosoft.com>;
> wei.liu@...nel.org; Malcolm Bumgardner (mbumgard)
> <mbumgard@...co.com>; Umesha G M (ugm) <ugm@...co.com>; Niranjan M
> M (nimm) <nimm@...co.com>; xe-linux-external(mailer list) <xe-linux-
> external@...co.com>; kuba@...nel.org; linux-hyperv@...r.kernel.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: Re: [PATCH v3] net: hyperv: add support for vlans in netvsc driver
> 
> 
> 
> On 21/07/20, 4:57 AM, "David Miller" <davem@...emloft.net> wrote:
> 
>     From: Sriram Krishnan <srirakr2@...co.com>
>     Date: Mon, 20 Jul 2020 22:15:51 +0530
> 
>     > +	if (skb->protocol == htons(ETH_P_8021Q)) {
>     > +		u16 vlan_tci = 0;
>     > +		skb_reset_mac_header(skb);
> 
>    > Please place an empty line between basic block local variable declarations
>    > and actual code.
> 
>     > +				netdev_err(net,"Pop vlan err %x\n",pop_err);
> 
>     > A space is necessary before "pop_err".
> 
> Consolidated list of comments addressed:
> > 1. Blank line between declaration and code.
> Done
> 
> > 2. Error handling is different than other parts of this code.
> >   probably just need a goto drop on error.
> Done
> 
> > It seems like you are putting into message, then driver is putting it
> > into meta-data in next code block. Maybe it should be combined?
> Not done
> This was on purpose. Merging the two code blocks might break existing
> functionality.
> There could be other modes where the packet arrives with 802.1q already in
> the Skb and the skb->protocol needn’t be 802.1q.
> 
> > packet->total_bytes should be updated too.
> Not done.
> The total_bytes needs be the total length of packet after the host OS adds the
> 802.1q header back in before tx. Updating the total_bytes to -= VLAN_HEADER
> will lead to packet drop in the Host OS driver.

If you make this change, did you see any drop in a live test? The
"packet->total_bytes" in struct hv_netvsc_packet  is for book keeping 
only, which is used for stats info, and not visible by the host at all.

I made this suggestion because the "regular" vlan packet length was 
counted by bytes without the VLAN_HLEN(4) -- the vlan tag is 
in the skb metadata, separately from the ethernet header. I want the 
statistical data for AF_PACKET mode consistent with the regular case.

struct hv_netvsc_packet {
        /* Bookkeeping stuff */
        u8 cp_partial; /* partial copy into send buffer */

        u8 rmsg_size; /* RNDIS header and PPI size */
        u8 rmsg_pgcnt; /* page count of RNDIS header and PPI */
        u8 page_buf_cnt;

        u16 q_idx;
        u16 total_packets;

        u32 total_bytes;
        u32 send_buf_index;
        u32 total_data_buflen;
};

Thanks
- Haiyang