netdev - Re: [PATCH RFC net-next] net: vlan/macvlan: count packets properly with gso

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]

Date:   Thu, 29 Nov 2018 21:13:34 +0000
From:   "Duyck, Alexander H" <alexander.h.duyck@...el.com>
To:     "dbanerje@...mai.com" <dbanerje@...mai.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH RFC net-next] net: vlan/macvlan: count packets properly
 with gso

On Thu, 2018-11-29 at 15:58 -0500, Debabrata Banerjee wrote:
> Fix packet count when using vlan/macvlan drivers with gso. Without this it
> is not possible to reconcile packet counts between underlying devices and
> these virtual devices. Additionally, the output looks wrong in a standalone
> way i.e. device MTU of 1500, 1 packet sent, 31856 bytes sent.
> 
> There are many other drivers that likely have a similar problem, although
> it is not clear how many of those could be used with gso. Perhaps all
> packet counting should be wrapped in a helper fn.
> 
> Signed-off-by: Debabrata Banerjee <dbanerje@...mai.com>
> ---
>  drivers/net/macvlan.c | 2 +-
>  net/8021q/vlan_core.c | 2 +-
>  net/8021q/vlan_dev.c  | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
> index fc8d5f1ee1ad..15e67a87f202 100644
> --- a/drivers/net/macvlan.c
> +++ b/drivers/net/macvlan.c
> @@ -566,7 +566,7 @@ static netdev_tx_t macvlan_start_xmit(struct sk_buff *skb,
>  
>  		pcpu_stats = this_cpu_ptr(vlan->pcpu_stats);
>  		u64_stats_update_begin(&pcpu_stats->syncp);
> -		pcpu_stats->tx_packets++;
> +		pcpu_stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  		pcpu_stats->tx_bytes += len;
>  		u64_stats_update_end(&pcpu_stats->syncp);
>  	} else {
> diff --git a/net/8021q/vlan_core.c b/net/8021q/vlan_core.c
> index a313165e7a67..e85f6665d0ed 100644
> --- a/net/8021q/vlan_core.c
> +++ b/net/8021q/vlan_core.c
> @@ -62,7 +62,7 @@ bool vlan_do_receive(struct sk_buff **skbp)
>  	rx_stats = this_cpu_ptr(vlan_dev_priv(vlan_dev)->vlan_pcpu_stats);
>  
>  	u64_stats_update_begin(&rx_stats->syncp);
> -	rx_stats->rx_packets++;
> +	rx_stats->rx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  	rx_stats->rx_bytes += skb->len;
>  	if (skb->pkt_type == PACKET_MULTICAST)
>  		rx_stats->rx_multicast++;
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index b2d9c8f27cd7..b28e7535a0b9 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -135,7 +135,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
>  
>  		stats = this_cpu_ptr(vlan->vlan_pcpu_stats);
>  		u64_stats_update_begin(&stats->syncp);
> -		stats->tx_packets++;
> +		stats->tx_packets += max_t(u16, 1, skb_shinfo(skb)->gso_segs);
>  		stats->tx_bytes += len;
>  		u64_stats_update_end(&stats->syncp);
>  	} else {

Instead of just checking for the max it might make more sense to do a
check using skb_is_gso, and then if true use gso_segs, otherwise just
default to 1.

Also your bytes are going to be totally messed up as well since the
headers are trimmed in the GSO frames. It might be worthwhile to just
have a branch based on skb_is_gso that sets the packets and bytes based
on the GSO values, and one that sets it for the default case.

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (3282 bytes)