Messages in this thread | | | Date | Wed, 13 Apr 2022 12:00:03 +0200 | From | Greg KH <> | Subject | Re: [PATCH] binder: Address corner cases in deferred copy and fixup |
| |
On Wed, Apr 13, 2022 at 10:54:27AM +0200, Alessandro Astone wrote: > When handling BINDER_TYPE_FDA object we are pushing a parent fixup > with a certain skip_size but no scatter-gather copy object, since > the copy is handled standalone. > If BINDER_TYPE_FDA is the last children the scatter-gather copy > loop will never stop to skip it, thus we are left with an item in > the parent fixup list. This will trigger the BUG_ON(). > > Furthermore, it is possible to receive BINDER_TYPE_FDA object > with num_fds=0 which will confuse the scatter-gather code. > > In the android userspace I could only find these usecases in the > libstagefright OMX implementation, so it might be that they're > doing something very weird, but nonetheless the kernel should not > panic about it. > > Fixes: 09184ae9b575 ("binder: defer copies of pre-patched txn data") > Signed-off-by: Alessandro Astone <ales.astone@gmail.com> > --- > drivers/android/binder.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/android/binder.c b/drivers/android/binder.c > index 8351c5638880..18ad6825ba30 100644 > --- a/drivers/android/binder.c > +++ b/drivers/android/binder.c > @@ -2295,7 +2295,7 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc, > { > int ret = 0; > struct binder_sg_copy *sgc, *tmpsgc; > - struct binder_ptr_fixup *pf = > + struct binder_ptr_fixup *tmppf, *pf =
Just make this a new line: struct binder_ptr_fixup *tmppf; above the existing line.
> list_first_entry_or_null(pf_head, struct binder_ptr_fixup, > node); > > @@ -2349,7 +2349,11 @@ static int binder_do_deferred_txn_copies(struct binder_alloc *alloc, > list_del(&sgc->node); > kfree(sgc); > } > - BUG_ON(!list_empty(pf_head));
So you are hitting this BUG_ON() today?
> + list_for_each_entry_safe(pf, tmppf, pf_head, node) { > + BUG_ON(pf->skip_size == 0); > + list_del(&pf->node); > + kfree(pf); > + } > BUG_ON(!list_empty(sgc_head)); > > return ret > 0 ? -EINVAL : ret; > @@ -2486,6 +2490,9 @@ static int binder_translate_fd_array(struct list_head *pf_head, > struct binder_proc *proc = thread->proc; > int ret; > > + if (fda->num_fds == 0) > + return 0;
Why return 0?
This feels like a separate issue from above, should this be 2 different commits?
thanks,
greg k-h
| |