Joel Fernandes wrote:
> I am sorry, what has changed in the updated patch? Please send clear diff
> notes in your patches or at least mention exactly what changed since previous
> patch revision. It is very difficult to review if you don't even mention what
> changed since previous revision. Please resend the patches again.
>
> Also I am OK with assuming small alloc success, so we are on the same page
> about that.
>
> One more comment below..
>
> Thanks!
>
> > @@ -722,10 +719,17 @@ static int ashmem_pin_unpin(struct ashmem_area *asma,
> > unsigned long cmd,
> > struct ashmem_pin pin;
> > size_t pgstart, pgend;
> > int ret = -EINVAL;
> > + struct ashmem_range *range = NULL;
> >
> > if (copy_from_user(&pin, p, sizeof(pin)))
> > return -EFAULT;
> >
> > + if (cmd == ASHMEM_PIN || cmd == ASHMEM_UNPIN) {
> > + range = kmem_cache_zalloc(ashmem_range_cachep, GFP_KERNEL);
> > + if (!range)
> > + return -ENOMEM;
>
> According to the too-small-to-fail rule, why are you checking for errors
> here?
The "too small to fail" memory-allocation rule is not __GFP_NOFAIL; it is
almost __GFP_NOFAIL.
Small __GFP_NOFAIL allocation might fail if current thread was killed by the
OOM killer or
memory allocation fault injection forced it to fail. And syzbot is finding many
bugs (using
memory allocation fault injection) where the caller is not checking for
allocation failures.
The old patch tried to allow syzbot to test this module by using __GFP_NOFAIL
(i.e. minimal
change), and the new patch is trying to allow syzbot to test this module by
moving
small __GFP_KERNEL allocation out of ashmem_mutex (i.e. cleaner change).
>
> > + }
> > +
> > mutex_lock(&ashmem_mutex);
> > wait_event(ashmem_shrink_wait, !atomic_read(&ashmem_shrink_inflight));
> >
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
