netdev - Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

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: <b5aed6412c3eee1c34a3b872d8e18c47deb9179f.camel@redhat.com>
Date:   Tue, 23 Oct 2018 09:59:08 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Subash Abhinov Kasiviswanathan <subashab@...eaurora.org>
Cc:     netdev@...r.kernel.org, Willem de Bruijn <willemb@...gle.com>,
        Steffen Klassert <steffen.klassert@...unet.com>
Subject: Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

Hi,

On Mon, 2018-10-22 at 13:04 -0600, Subash Abhinov Kasiviswanathan
wrote:
> 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@...hat.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)

Yes, thanks.

(also Williem already noted the above)

> > 
> > +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?

Not required, but currently helpful IMHO, as we should hit the above
only on unlikey and really unwonted configuration.

Note that only SKB_GSO_UDP_L4 GSO packets will not match the above
likely condition.

> 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.

Really? Is there any HW creating SKB_GSO_UDP_L4 packets on RX? if the
HW is doing that, without this patch, I think it's breaking existing
applications (which may expext that the read UDP frame length
implicitly describe the application level message length).

Cheers,

Paolo