netdev - RE: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to bpf

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]

Message-ID: <733bf731f95940a495830f3997307eaf@AcuMS.aculab.com>
Date:   Fri, 29 May 2020 12:41:38 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Eric Dumazet' <eric.dumazet@...il.com>,
        Dmitry Yakunin <zeil@...dex-team.ru>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "brakmo@...com" <brakmo@...com>
CC:     "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH bpf-next] bpf: add SO_KEEPALIVE and related options to
 bpf_setsockopt

From: Eric Dumazet
> Sent: 27 May 2020 17:43
> 
> On 5/27/20 8:05 AM, Dmitry Yakunin wrote:
> > This patch adds support of SO_KEEPALIVE flag and TCP related options
> > to bpf_setsockopt() routine. This is helpful if we want to enable or tune
> > TCP keepalive for applications which don't do it in the userspace code.
> > In order to avoid copy-paste, common code from classic setsockopt was moved
> > to auxiliary functions in the headers.
> 
> 
> Please split this in two patches :
> - one adding the helpers, a pure TCP patch.
> - one for BPF additions.
> 
...
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index a6fc234..1035e43 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
...
> > +			case TCP_KEEPIDLE:
> > +				if (val < 1 || val > MAX_TCP_KEEPIDLE)
> > +					ret = -EINVAL;
> > +				else
> > +					keepalive_time_set(tp, val);
> > +				break;
> > +			case TCP_KEEPINTVL:
> > +				if (val < 1 || val > MAX_TCP_KEEPINTVL)
> > +					ret = -EINVAL;
> > +				else
> > +					tp->keepalive_intvl = val * HZ;
> > +				break;
> > +			case TCP_KEEPCNT:
> > +				if (val < 1 || val > MAX_TCP_KEEPCNT)
> > +					ret = -EINVAL;
> > +				else
> > +					tp->keepalive_probes = val;
> > +				break;
> > +			case TCP_SYNCNT:
> > +				if (val < 1 || val > MAX_TCP_SYNCNT)
> > +					ret = -EINVAL;
> > +				else
> > +					icsk->icsk_syn_retries = val;
> > +				break;
> > +			case TCP_USER_TIMEOUT:
> > +				if (val < 0)
> > +					ret = -EINVAL;
> > +				else
> > +					icsk->icsk_user_timeout = val;
> > +				break;

It also cannot be right to be layer breaking like this
and directly accessing the protocol socket internals.

At least a kernel_setsockopt() function keeps this separate
and ensures that the socket type is correct.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)