Re: [PATCH 12/33] userfaultfd: non-cooperative: Add madvise() event for MADV_DONTNEED requestg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Nov 03, 2016 at 11:24:46AM -0600, Mike Rapoport wrote:
> (changed 'CC:
> - Michael Rapoport <RAPOPORT@xxxxxxxxxx>,
> - Dr. David Alan Gilbert@v2.random,  <dgilbert@xxxxxxxxxx>,
> + Dr. David Alan Gilbert  <dgilbert@xxxxxxxxxx>,
> - Pavel Emelyanov <xemul@xxxxxxxxxxxxx>@v2.random
> + Pavel Emelyanov <xemul@xxxxxxxxxxxxx>

Sorry for this mess, so it turns out git will crunch a non rfc2822
compliant email address just fine, but postfix will not be happy and
it rewrites the header in a best effort way. The email is still
delivered because send-email specifies the addresses that git can cope
with on the sendmail command line instead of using -t, that's why the
email is delivered by the header is garbled.

On the git list they're discussing if the parsing of the email
addresses can be made more strict to follow rfc2822, otherwise from
--dry-run things look ok, but then when you removed --dry-run you find
out the hard way you left a trailing " in an email address...

> On Thu, Nov 03, 2016 at 04:01:12PM +0800, Hillf Danton wrote:
> > On Thursday, November 03, 2016 3:34 AM Andrea Arcangeli wrote:
> > > +void madvise_userfault_dontneed(struct vm_area_struct *vma,
> > > +				struct vm_area_struct **prev,
> > > +				unsigned long start, unsigned long end)
> > > +{
> > > +	struct userfaultfd_ctx *ctx;
> > > +	struct userfaultfd_wait_queue ewq;
> > > +
> > > +	ctx = vma->vm_userfaultfd_ctx.ctx;
> > > +	if (!ctx || !(ctx->features & UFFD_FEATURE_EVENT_MADVDONTNEED))
> > > +		return;
> > > +
> > > +	userfaultfd_ctx_get(ctx);
> > > +	*prev = NULL; /* We wait for ACK w/o the mmap semaphore */
> > > +	up_read(&vma->vm_mm->mmap_sem);
> > > +
> > > +	msg_init(&ewq.msg);
> > > +
> > > +	ewq.msg.event = UFFD_EVENT_MADVDONTNEED;
> > > +	ewq.msg.arg.madv_dn.start = start;
> > > +	ewq.msg.arg.madv_dn.end = end;
> > > +
> > > +	userfaultfd_event_wait_completion(ctx, &ewq);
> > > +
> > > +	down_read(&vma->vm_mm->mmap_sem);
> > 
> > After napping with mmap_sem released, is vma still valid?

Wow, nice catch Hillf. There was zero chance to catch this at runtime,
we don't munmap the vma while the testcase runs, plus even if we did
such thing, to notice it would need to be reused fast enough. It was
just a single instruction window for a pointer dereference...

> You are right, vma may be invalid at that point. Thanks for spotting.
> 
> Andrea, how do you prefer the fix, incremental or the entire patch updated?

I'm applying your updated patch, fix you sent is correct.

I will also move *prev= NULL just after up_read too, doing it before
up_read looks like it has to be done before before releasing the lock
which is not the case. Furthermore it's a microoptimization for
scalability to do it after, but it won't make any runtime difference
of course.

Thanks,
Andrea

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]