On Mon, Nov 28, 2016 at 04:01:14PM +0100, Martin Pieuchot wrote:
> @@ -313,7 +319,13 @@ bpf_detachd(struct bpf_d *d)
> int error;
>
> d->bd_promisc = 0;
> +
> + bpf_get(d);
> + mtx_leave(&d->bd_mtx);
> error = ifpromisc(bp->bif_ifp, 0);
> + mtx_enter(&d->bd_mtx);
> + bpf_put(d);
> +
If I see something like this, I ask myself wether bp has to be
protected by the mutex like this:
bpf_get(d);
ifp = bp->bif_ifp;
mtx_leave(&d->bd_mtx);
error = ifpromisc(ifp, 0);
mtx_enter(&d->bd_mtx);
bpf_put(d);
But I think your code is safe as bd_bif is only modified with kernel
lock.
> @@ -542,6 +568,7 @@ bpf_wakeup_cb(void *xd)
>
> selwakeup(&d->bd_sel);
> bpf_put(d);
> +
> }
>
> int
This new line is wrong.
> @@ -717,7 +753,9 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> *(u_int *)addr = size = bpf_maxbufsize;
> else if (size < BPF_MINBUFSIZE)
> *(u_int *)addr = size = BPF_MINBUFSIZE;
> + mtx_enter(&d->bd_mtx);
> d->bd_bufsize = size;
> + mtx_leave(&d->bd_mtx);
> }
> break;
>
bd_bufsize needs protection, but you only add it in BIOCSBLEN,
you should also put a mutex in BIOCGBLEN.
> @@ -778,30 +815,36 @@ bpfioctl(dev_t dev, u_long cmd, caddr_t
> * Get device parameters.
> */
> case BIOCGDLT:
> + mtx_enter(&d->bd_mtx);
> if (d->bd_bif == NULL)
> error = EINVAL;
> else
> *(u_int *)addr = d->bd_bif->bif_dlt;
> + mtx_leave(&d->bd_mtx);
> break;
bd_bif is not protected elsewhere. bif_dlt is only set during attach.
So I think you need no mutex here.
>
> /*
> * Set device parameters.
> */
> case BIOCSDLT:
> + mtx_enter(&d->bd_mtx);
> if (d->bd_bif == NULL)
> error = EINVAL;
> else
> error = bpf_setdlt(d, *(u_int *)addr);
> + mtx_leave(&d->bd_mtx);
> break;
bd_bif needs not protection, so put the mutex only in the else path.
Then it is obvious why it is there.
>
> /*
> * Set interface name.
> */
> case BIOCGETIF:
> + mtx_enter(&d->bd_mtx);
> if (d->bd_bif == NULL)
> error = EINVAL;
> else
> bpf_ifname(d->bd_bif->bif_ifp, (struct ifreq *)addr);
> + mtx_leave(&d->bd_mtx);
> break;
The interface name is not used by bpf without kernel lock. You do
not need a mutex here.
> @@ -1151,6 +1197,8 @@ filt_bpfread(struct knote *kn, long hint
> {
> struct bpf_d *d = kn->kn_hook;
>
> + KERNEL_ASSERT_LOCKED();
> +
> kn->kn_data = d->bd_hlen;
> if (d->bd_immediate)
> kn->kn_data += d->bd_slen;
You need the mutex here to access bd_slen.
> @@ -1232,12 +1279,10 @@ _bpf_mtap(caddr_t arg, const struct mbuf
> if (!gottime++)
> microtime(&tv);
>
> - KERNEL_LOCK();
> - s = splnet();
> + mtx_enter(&d->bd_mtx);
> bpf_catchpacket(d, (u_char *)m, pktlen, slen, cpfn,
> &tv);
> - splx(s);
> - KERNEL_UNLOCK();
> + mtx_leave(&d->bd_mtx);
>
> if (d->bd_fildrop)
> drop = 1;
bd_fildrop can be changed by ioctl. Can we expect to read the
correct value without a memory barrier?
Same question applies to bd_dirfilt.
bd_rcount is a long value incremented atomically in _bpf_mtap().
It is read from an other CPU with kernel lock during an ioctl. Is
this sufficient to get a consisten value? Do we also need an atomic
operation when reading it?
> --- net/bpfdesc.h 22 Aug 2016 10:40:36 -0000 1.31
> +++ net/bpfdesc.h 28 Nov 2016 14:51:06 -0000
> @@ -56,15 +56,17 @@ struct bpf_d {
> * fbuf (free) - When read is done, put cluster here.
> * On receiving, if sbuf is full and fbuf is 0, packet is dropped.
> */
> + struct mutex bd_mtx; /* protect buffer slots below */
> caddr_t bd_sbuf; /* store slot */
> caddr_t bd_hbuf; /* hold slot */
> caddr_t bd_fbuf; /* free slot */
> int bd_slen; /* current length of store buffer */
> int bd_hlen; /* current length of hold buffer */
> -
> int bd_bufsize; /* absolute length of buffers */
So bd_mtx is protecting the fields above. Everything below must
either be not accessed without kernel lock, or only be set during
attach. Is that correct or do I have a wrong understanding of the
SMP model?
>
> - struct bpf_if * bd_bif; /* interface descriptor */
> + int __in_uiomove;
Why do you prefix it with __ ? bd_uiomove would be more consistent.
bluhm
