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] [day] [month] [year] [list]
Date:   Tue, 19 Feb 2019 22:31:37 +0000
From:   Lawrence Brakmo <brakmo@...com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        netdev <netdev@...r.kernel.org>
CC:     Martin Lau <kafai@...com>, Alexei Starovoitov <ast@...com>,
        "Daniel Borkmann" <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next 7/9] bpf: Sample NRM BPF program to limit egress
 bw

On 2/19/19, 10:29 AM, "netdev-owner@...r.kernel.org on behalf of Eric Dumazet" <netdev-owner@...r.kernel.org on behalf of eric.dumazet@...il.com> wrote:
    
    
    On 02/18/2019 09:38 PM, brakmo wrote:
    
    > +
    > +static __always_inline void get_nrm_pkt_info(struct bpf_sock *sk,
    > +					     struct nrm_pkt_info *pkti)
    > +{
    > +	if (sk->family == AF_INET6 || sk->family == AF_INET) {
    > +		pkti->is_ip = true;
    > +		pkti->is_tcp = (sk->protocol == IPPROTO_TCP);
    > +		if (pkti->is_tcp) {
    > +			struct bpf_tcp_sock *tp;
    > +
    > +			tp = bpf_tcp_sock(sk);
    > +			if (tp)
    > +				pkti->ecn = tp->ecn_flags & TCP_ECN_OK;
    > +			else
    > +				pkti->ecn = 0;
    > +		} else {
    > +			pkti->ecn = 0;
    > +		}
    > +	} else {
    > +		pkti->is_ip = false;
    > +		pkti->is_tcp = false;
    > +		pkti->ecn = 0;
    > +	}
    > +}
    > 
    
    This looks very strange.
    
    ECN capability is per packet, and does not need access to the original
    TCP socket really.

    We definitely can use ECN with UDP packets.
    
    IMO this sample looks like a work in progress.

This sample NRM program focuses on TCP, I should have made that more explicit. I originally was checking the ECN bits on the packet, but someone pointed out that since I was focusing on TCP, it would be more efficient to just look at the TCP state (saving having to read the packet header). However, your point is correct in that I could be wrongly marking packets that I should not mark, such as pure ACKs. I will go back to the previous version that looks at the ECN bits in the header and not limit ECN marking to just TCP.
    
    EDT model allows to implement full shaping (not only virtual one)
    by twaking/advancing skb->tstamp 
    (see commit f11216b24219a bpf: add skb->tstamp r/w access from tc clsact and cg skb progs)
    
I have a version of an NRM BPF program that uses EDT, so I know what you mean. However, I decided to send it as a separate patch (including an ingress sample program).

    Implementing shaping with the need of accessing TCP sockets seems a layering violation,
    a lot of things can go wrong with this model.
    For instance, you wont be able to offload this.

On the other hand, there are also advantages to having access to the TCP socket. For example, one has more tools to affect the TCP rate (like the proposed helper bpf_tcp_enter_cwr() which does not require dropping the packet). We will need more experience to fully understand the tradeoffs between the various implementations of rate limiting and/or shaping.

Thank you for your feedback.

    
    

Powered by blists - more mailing lists