svn commit: r348303 - head/sys/net

[Gleb Smirnoff]

  Hi Andrey,

I made a different change to mitigate this panic: don't clear the pointer.

--- a/FreeBSD/sys/net/bpf.c
+++ b/FreeBSD/sys/net/bpf.c
@@ -857,7 +857,6 @@ bpf_detachd_locked(struct bpf_d *d, bool detached_ifp)
        /* Save bd_writer value */
        error = d->bd_writer;
        ifp = bp->bif_ifp;
-       d->bd_bif = NULL;
        if (detached_ifp) {
                /*
                 * Notify descriptor as it's detached, so that any

Since every bpf_d holds a reference on bpf_if until delayed free happens,
the the bpf_if is going to be valid.

This allows not to use epoch_wait and run fully async. The patch above is
a minimal patch: with NULL assignment removed, several more pieces of code
can be removed in bpf.c

Of course your patch also is going to work, but what do you think:
are there any landmines with fully async approach?

On Mon, May 27, 2019 at 12:41:41PM +0000, Andrey V. Elsukov wrote:
A> Author: ae
A> Date: Mon May 27 12:41:41 2019
A> New Revision: 348303
A> URL: https://svnweb.freebsd.org/changeset/base/348303
A> 
A> Log:
A>   Fix possible NULL pointer dereference.
A>   
A>   bpf_mtap() can invoke catchpacket() for already detached descriptor.
A>   And this can lead to NULL pointer dereference, since bd_bif pointer
A>   was reset to NULL in bpf_detachd_locked(). To avoid this, use
A>   NET_EPOCH_WAIT() when descriptor is removed from interface's descriptors
A>   list. After the wait it is safe to modify descriptor's content.
A>   
A>   Submitted by:	kib
A>   Reported by:	slavash
A>   MFC after:	1 week
A> 
A> Modified:
A>   head/sys/net/bpf.c
A> 
A> Modified: head/sys/net/bpf.c
A> ==============================================================================
A> --- head/sys/net/bpf.c	Mon May 27 06:37:23 2019	(r348302)
A> +++ head/sys/net/bpf.c	Mon May 27 12:41:41 2019	(r348303)
A> @@ -850,10 +850,15 @@ bpf_detachd_locked(struct bpf_d *d, bool detached_ifp)
A>  	/* Check if descriptor is attached */
A>  	if ((bp = d->bd_bif) == NULL)
A>  		return;
A> +	/*
A> +	 * Remove d from the interface's descriptor list.
A> +	 * And wait until bpf_[m]tap*() will finish their possible work
A> +	 * with descriptor.
A> +	 */
A> +	CK_LIST_REMOVE(d, bd_next);
A> +	NET_EPOCH_WAIT();
A>  
A>  	BPFD_LOCK(d);
A> -	/* Remove d from the interface's descriptor list. */
A> -	CK_LIST_REMOVE(d, bd_next);
A>  	/* Save bd_writer value */
A>  	error = d->bd_writer;
A>  	ifp = bp->bif_ifp;
A> 

-- 
Gleb Smirnoff