Re: svn commit: r348303 - head/sys/net
On Wed, May 29, 2019 at 08:56:23PM +0300, Andrey V. Elsukov wrote: A> On 29.05.2019 06:12, Gleb Smirnoff wrote: A> > A> bpf_mtap() is not the only consumer of bd_bif, some of them expect it A> > A> becomes NULL when descriptor is detached. A> > A> > May be then make a flag attached/detached? A> A> Do you have benchmark results that show some benefits in performance? :) A> I prefer to wait some time after MFC to get a bit wide testing, before A> doing another performance optimizations. I was mostly motivated not by performance but by design, so that it conforms to delayed free architecture. Btw, now I see that my one liner isn't sufficient, since we bpfif_rele() not in the deferred context but right after CK_LIST_REMOVE(). -- Gleb Smirnoff ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r348303 - head/sys/net
On 29.05.2019 06:12, Gleb Smirnoff wrote: > A> bpf_mtap() is not the only consumer of bd_bif, some of them expect it > A> becomes NULL when descriptor is detached. > > May be then make a flag attached/detached? Do you have benchmark results that show some benefits in performance? :) I prefer to wait some time after MFC to get a bit wide testing, before doing another performance optimizations. -- WBR, Andrey V. Elsukov signature.asc Description: OpenPGP digital signature
Re: svn commit: r348303 - head/sys/net
On Wed, May 29, 2019 at 03:23:23AM +0300, Andrey V. Elsukov wrote:
A> > --- a/FreeBSD/sys/net/bpf.c
A> > +++ b/FreeBSD/sys/net/bpf.c
A> > @@ -857,7 +857,6 @@ bpf_detachd_locked(struct bpf_d *d, bool detached_ifp)
A> > /* Save bd_writer value */
A> > error = d->bd_writer;
A> > ifp = bp->bif_ifp;
A> > - d->bd_bif = NULL;
A> > if (detached_ifp) {
A> > /*
A> > * Notify descriptor as it's detached, so that any
A> >
A> > Since every bpf_d holds a reference on bpf_if until delayed free happens,
A> > the the bpf_if is going to be valid.
A> >
A> > This allows not to use epoch_wait and run fully async. The patch above is
A> > a minimal patch: with NULL assignment removed, several more pieces of code
A> > can be removed in bpf.c
A> >
A> > Of course your patch also is going to work, but what do you think:
A> > are there any landmines with fully async approach?
A>
A> Hi,
A>
A> bpf_mtap() is not the only consumer of bd_bif, some of them expect it
A> becomes NULL when descriptor is detached.
May be then make a flag attached/detached?
--
Gleb Smirnoff
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r348303 - head/sys/net
29.05.2019 3:10, 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?
Hi,
bpf_mtap() is not the only consumer of bd_bif, some of them expect it
becomes NULL when descriptor is detached.
--
WBR, Andrey V. Elsukov
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r348303 - head/sys/net
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 +, 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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r348303 - head/sys/net
Author: ae Date: Mon May 27 12:41:41 2019 New Revision: 348303 URL: https://svnweb.freebsd.org/changeset/base/348303 Log: Fix possible NULL pointer dereference. bpf_mtap() can invoke catchpacket() for already detached descriptor. And this can lead to NULL pointer dereference, since bd_bif pointer was reset to NULL in bpf_detachd_locked(). To avoid this, use NET_EPOCH_WAIT() when descriptor is removed from interface's descriptors list. After the wait it is safe to modify descriptor's content. Submitted by: kib Reported by: slavash MFC after:1 week Modified: head/sys/net/bpf.c Modified: head/sys/net/bpf.c == --- head/sys/net/bpf.c Mon May 27 06:37:23 2019(r348302) +++ head/sys/net/bpf.c Mon May 27 12:41:41 2019(r348303) @@ -850,10 +850,15 @@ bpf_detachd_locked(struct bpf_d *d, bool detached_ifp) /* Check if descriptor is attached */ if ((bp = d->bd_bif) == NULL) return; + /* +* Remove d from the interface's descriptor list. +* And wait until bpf_[m]tap*() will finish their possible work +* with descriptor. +*/ + CK_LIST_REMOVE(d, bd_next); + NET_EPOCH_WAIT(); BPFD_LOCK(d); - /* Remove d from the interface's descriptor list. */ - CK_LIST_REMOVE(d, bd_next); /* Save bd_writer value */ error = d->bd_writer; ifp = bp->bif_ifp; ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
