From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: Prepare bpf_sysctl() for net_sysctl() unlocking
To: Vitaliy Makkoveev <mvs@openbsd.org>
Cc: tech@openbsd.org
Date: Mon, 12 Aug 2024 16:07:05 +0200

On Sun, Aug 11, 2024 at 07:23:13PM +0300, Vitaliy Makkoveev wrote:
> Anyone? Protocol sysctl(2) locking is discussible, but statistics
> is per-CUP counters and could be received lockless, so partial
> net_sysctl() unlocking makes sense.

OK bluhm@

> > On 6 Aug 2024, at 18:40, Vitaliy Makkoveev <mvs@openbsd.org> wrote:
> > 
> > On Tue, Aug 06, 2024 at 03:30:01PM +0300, Vitaliy Makkoveev wrote:
> >> bluhm@ reworked sysctl_int_bounded() to deal with atomically accessed
> >> integer variables. bpf(4) sysctl has two such variables, NET_BPF_BUFSIZE
> >> and NET_BPF_MAXBUFSIZE respectively, so rework bpf_sysctl() to be ready
> >> for upcoming net_sysctl() unlocking.
> >> 
> >> Please note, bpf_movein() obviously require to store `bpf_maxbufsize'
> >> to local variable, but bpf_sysctl() accesses it only once, so
> >> atomic_load_int() is unnecessary here. This is also true for
> >> `bpf_bufsize' which is accessed only once in bpfopen().
> >> 
> >> ok?
> >> 
> > 
> > Use atomic_load_int(9) instead direct access.
> > 
> > Index: sys/net/bpf.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/bpf.c,v
> > diff -u -p -r1.223 bpf.c
> > --- sys/net/bpf.c	5 Aug 2024 23:56:10 -0000	1.223
> > +++ sys/net/bpf.c	6 Aug 2024 15:39:52 -0000
> > @@ -84,10 +84,15 @@
> > #define PRINET  26			/* interruptible */
> > 
> > /*
> > + * Locks used to protect data:
> > + *	a	atomic
> > + */
> > +
> > +/*
> >  * The default read buffer size is patchable.
> >  */
> > -int bpf_bufsize = BPF_BUFSIZE;
> > -int bpf_maxbufsize = BPF_MAXBUFSIZE;
> > +int bpf_bufsize = BPF_BUFSIZE;		/* [a] */
> > +int bpf_maxbufsize = BPF_MAXBUFSIZE;	/* [a] */
> > 
> > /*
> >  *  bpf_iflist is the list of interfaces; each corresponds to an ifnet
> > @@ -117,8 +122,6 @@ int	filt_bpfread(struct knote *, long);
> > int	filt_bpfreadmodify(struct kevent *, struct knote *);
> > int	filt_bpfreadprocess(struct knote *, struct kevent *);
> > 
> > -int	bpf_sysctl_locked(int *, u_int, void *, size_t *, void *, size_t);
> > -
> > struct bpf_d *bpfilter_lookup(int);
> > 
> > /*
> > @@ -137,9 +140,6 @@ void	bpf_d_smr(void *);
> > void	bpf_get(struct bpf_d *);
> > void	bpf_put(struct bpf_d *);
> > 
> > -
> > -struct rwlock bpf_sysctl_lk = RWLOCK_INITIALIZER("bpfsz");
> > -
> > int
> > bpf_movein(struct uio *uio, struct bpf_d *d, struct mbuf **mp,
> >     struct sockaddr *sockp)
> > @@ -393,7 +393,7 @@ bpfopen(dev_t dev, int flag, int mode, s
> > 
> > 	/* Mark "free" and do most initialization. */
> > 	bd->bd_unit = unit;
> > -	bd->bd_bufsize = bpf_bufsize;
> > +	bd->bd_bufsize = atomic_load_int(&bpf_bufsize);
> > 	bd->bd_sig = SIGIO;
> > 	mtx_init(&bd->bd_mtx, IPL_NET);
> > 	task_set(&bd->bd_wake_task, bpf_wakeup_cb, bd);
> > @@ -853,9 +853,11 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t 
> > 			error = EINVAL;
> > 		else {
> > 			u_int size = *(u_int *)addr;
> > +			int bpf_maxbufsize_local =
> > +			    atomic_load_int(&bpf_maxbufsize);
> > 
> > -			if (size > bpf_maxbufsize)
> > -				*(u_int *)addr = size = bpf_maxbufsize;
> > +			if (size > bpf_maxbufsize_local)
> > +				*(u_int *)addr = size = bpf_maxbufsize_local;
> > 			else if (size < BPF_MINBUFSIZE)
> > 				*(u_int *)addr = size = BPF_MINBUFSIZE;
> > 			mtx_enter(&d->bd_mtx);
> > @@ -1815,42 +1817,25 @@ bpfsdetach(void *p)
> > }
> > 
> > int
> > -bpf_sysctl_locked(int *name, u_int namelen, void *oldp, size_t *oldlenp,
> > -    void *newp, size_t newlen)
> > +bpf_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
> > +    size_t newlen)
> > {
> > +	if (namelen != 1)
> > +		return (ENOTDIR);
> > +
> > 	switch (name[0]) {
> > 	case NET_BPF_BUFSIZE:
> > 		return sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> > -		    &bpf_bufsize, BPF_MINBUFSIZE, bpf_maxbufsize);
> > +		    &bpf_bufsize, BPF_MINBUFSIZE,
> > +		    atomic_load_int(&bpf_maxbufsize));
> > 	case NET_BPF_MAXBUFSIZE:
> > 		return sysctl_int_bounded(oldp, oldlenp, newp, newlen,
> > 		    &bpf_maxbufsize, BPF_MINBUFSIZE, INT_MAX);
> > 	default:
> > 		return (EOPNOTSUPP);
> > 	}
> > -}
> > -
> > -int
> > -bpf_sysctl(int *name, u_int namelen, void *oldp, size_t *oldlenp, void *newp,
> > -    size_t newlen)
> > -{
> > -	int flags = RW_INTR;
> > -	int error;
> > 
> > -	if (namelen != 1)
> > -		return (ENOTDIR);
> > -
> > -	flags |= (newp == NULL) ? RW_READ : RW_WRITE;
> > -
> > -	error = rw_enter(&bpf_sysctl_lk, flags);
> > -	if (error != 0)
> > -		return (error);
> > -
> > -	error = bpf_sysctl_locked(name, namelen, oldp, oldlenp, newp, newlen);
> > -
> > -	rw_exit(&bpf_sysctl_lk);
> > -
> > -	return (error);
> > +	/* NOTREACHED */
> > }
> > 
> > struct bpf_d *
>