From mboxrd@z Thu Jan  1 00:00:00 1970
From: Subash Abhinov Kasiviswanathan <subashab@codeaurora.org>
Subject: Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection
Date: Mon, 22 Oct 2018 13:04:36 -0600
Message-ID: <f87ef4ed8f5c0ab5989d3e067b218005@codeaurora.org>
References: <cover.1539957909.git.pabeni@redhat.com>
 <63e4ceb238db122d3d831f0809285243701b2284.1539957909.git.pabeni@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Cc: netdev@vger.kernel.org, Willem de Bruijn <willemb@google.com>,
        Steffen Klassert <steffen.klassert@secunet.com>
To: Paolo Abeni <pabeni@redhat.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from smtp.codeaurora.org ([198.145.29.96]:38506 "EHLO
        smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1728289AbeJWDYV (ORCPT
        <rfc822;netdev@vger.kernel.org>); Mon, 22 Oct 2018 23:24:21 -0400
In-Reply-To: <63e4ceb238db122d3d831f0809285243701b2284.1539957909.git.pabeni@redhat.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On 2018-10-19 08:25, Paolo Abeni wrote:
> In some scenarios, the GRO engine can assemble an UDP GRO packet
> that ultimately lands on a non GRO-enabled socket.
> This patch tries to address the issue explicitly checking for the UDP
> socket features before enqueuing the packet, and eventually segmenting
> the unexpected GRO packet, as needed.
> 
> We must also cope with re-insertion requests: after segmentation the
> UDP code calls the helper introduced by the previous patches, as 
> needed.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> +static inline bool udp_unexpected_gso(struct sock *sk, struct sk_buff
> *skb)
> +{
> +	return !udp_sk(sk)->gro_enabled && skb_is_gso(skb) &&
> +	       skb_shinfo(skb)->gso_type & SKB_GSO_UDP_L4;
> +}
> +
> +static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> +					      struct sk_buff *skb)
> +{
> +	struct sk_buff *segs;
> +
> +	/* the GSO CB lays after the UDP one, no need to save and restore
> any
> +	 * CB fragment, just initialize it
> +	 */
> +	segs = __skb_gso_segment(skb, NETIF_F_SG, false);
> +	if (unlikely(IS_ERR(segs)))
> +		kfree_skb(skb);
> +	else if (segs)
> +		consume_skb(skb);
> +	return segs;
> +}
> +
> +

Hi Paolo

Do we need to check for IS_ERR_OR_NULL(segs)

> 
> +void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int
> proto);
> +
> +static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct sk_buff *next, *segs;
> +	int ret;
> +
> +	if (likely(!udp_unexpected_gso(sk, skb)))
> +		return udp_queue_rcv_one_skb(sk, skb);

> +static int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> +{
> +	struct sk_buff *next, *segs;
> +	int ret;
> +
> +	if (likely(!udp_unexpected_gso(sk, skb)))
> +		return udpv6_queue_rcv_one_skb(sk, skb);
> +

Is the "likely" required here?
HW can coalesce all incoming streams of UDP and may not know the socket 
state.
In that case, a socket not having UDP GRO option might see a penalty 
here.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project