Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1700045 > unrolled thread

Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Andrea Arcangeli <aarcange@redhat.com> - 2017-07-31 15:40 +0200
    Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Michal Hocko <mhocko@kernel.org> - 2017-07-31 15:50 +0200
      Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-08-02 14:40 +0200
        Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone "Dr. David Alan Gilbert" <dgilbert@redhat.com> - 2017-08-02 15:30 +0200
        Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Andrea Arcangeli <aarcange@redhat.com> - 2017-08-02 18:00 +0200
          Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Michal Hocko <mhocko@kernel.org> - 2017-08-02 18:30 +0200
            Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Andrea Arcangeli <aarcange@redhat.com> - 2017-08-02 18:50 +0200
              Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Mike Rapoport <rppt@linux.vnet.ibm.com> - 2017-08-03 19:30 +0200
                Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone Andrea Arcangeli <aarcange@redhat.com> - 2017-08-03 23:30 +0200

#1700045 — Re: [PATCH] userfaultfd_zeropage: return -ENOSPC in case mm has gone

On Mon, Jul 31, 2017 at 02:22:04PM +0200, Michal Hocko wrote:
> On Thu 27-07-17 09:26:59, Mike Rapoport wrote:
> > In the non-cooperative userfaultfd case, the process exit may race with
> > outstanding mcopy_atomic called by the uffd monitor.  Returning -ENOSPC
> > instead of -EINVAL when mm is already gone will allow uffd monitor to
> > distinguish this case from other error conditions.
> 
> Normally we tend to return ESRCH in such case. ENOSPC sounds rather
> confusing...

This is in sync and consistent with the retval for UFFDIO_COPY upstream:

	if (mmget_not_zero(ctx->mm)) {
		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
				   uffdio_copy.len);
		mmput(ctx->mm);
	} else {
		return -ENOSPC;
	}

If you preferred ESRCH I certainly wouldn't have been against, but we
should have discussed it before it was upstream. All it matters is
it's documented in the great manpage that was written for it as quoted
below.

+.TP
+.B ENOENT
+(Since Linux 4.11)
+The faulting process has changed
+its virtual memory layout simultaneously with outstanding
+.I UFFDIO_COPY
+operation.
+.TP
+.B ENOSPC
+(Since Linux 4.11)
+The faulting process has exited at the time of
+.I UFFDIO_COPY
+operation.

To change it now, we would need to involve manpage and other code
changes.

Thanks,
Andrea

[toc] | [next] | [standalone]

#1700058

On Mon 31-07-17 15:32:47, Andrea Arcangeli wrote:
> On Mon, Jul 31, 2017 at 02:22:04PM +0200, Michal Hocko wrote:
> > On Thu 27-07-17 09:26:59, Mike Rapoport wrote:
> > > In the non-cooperative userfaultfd case, the process exit may race with
> > > outstanding mcopy_atomic called by the uffd monitor.  Returning -ENOSPC
> > > instead of -EINVAL when mm is already gone will allow uffd monitor to
> > > distinguish this case from other error conditions.
> > 
> > Normally we tend to return ESRCH in such case. ENOSPC sounds rather
> > confusing...
> 
> This is in sync and consistent with the retval for UFFDIO_COPY upstream:
> 
> 	if (mmget_not_zero(ctx->mm)) {
> 		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
> 				   uffdio_copy.len);
> 		mmput(ctx->mm);
> 	} else {
> 		return -ENOSPC;
> 	}
> 
> If you preferred ESRCH I certainly wouldn't have been against, but we
> should have discussed it before it was upstream. All it matters is
> it's documented in the great manpage that was written for it as quoted
> below.

OK, I wasn't aware of this.
 
> +.TP
> +.B ENOENT
> +(Since Linux 4.11)
> +The faulting process has changed
> +its virtual memory layout simultaneously with outstanding
> +.I UFFDIO_COPY
> +operation.
> +.TP
> +.B ENOSPC
> +(Since Linux 4.11)
> +The faulting process has exited at the time of
> +.I UFFDIO_COPY
> +operation.
> 
> To change it now, we would need to involve manpage and other code
> changes.

Well, ESRCH is more appropriate so I would rather change it sooner than
later. But if we are going to risk user space breakage then this is not
worth the risk. I expected there are very few users of this API
currently so maybe it won't be a big disaster?

Anyway, at least this is documented so I will leave the decision to you.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1702083

On Mon, Jul 31, 2017 at 03:45:08PM +0200, Michal Hocko wrote:
> On Mon 31-07-17 15:32:47, Andrea Arcangeli wrote:
> > On Mon, Jul 31, 2017 at 02:22:04PM +0200, Michal Hocko wrote:
> > > On Thu 27-07-17 09:26:59, Mike Rapoport wrote:
> > > > In the non-cooperative userfaultfd case, the process exit may race with
> > > > outstanding mcopy_atomic called by the uffd monitor.  Returning -ENOSPC
> > > > instead of -EINVAL when mm is already gone will allow uffd monitor to
> > > > distinguish this case from other error conditions.
> > > 
> > > Normally we tend to return ESRCH in such case. ENOSPC sounds rather
> > > confusing...
> > 
> > This is in sync and consistent with the retval for UFFDIO_COPY upstream:
> > 
> > 	if (mmget_not_zero(ctx->mm)) {
> > 		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
> > 				   uffdio_copy.len);
> > 		mmput(ctx->mm);
> > 	} else {
> > 		return -ENOSPC;
> > 	}
> > 
> > If you preferred ESRCH I certainly wouldn't have been against, but we
> > should have discussed it before it was upstream. All it matters is
> > it's documented in the great manpage that was written for it as quoted
> > below.
> 
> OK, I wasn't aware of this.
> 
> > +.TP
> > +.B ENOENT
> > +(Since Linux 4.11)
> > +The faulting process has changed
> > +its virtual memory layout simultaneously with outstanding
> > +.I UFFDIO_COPY
> > +operation.
> > +.TP
> > +.B ENOSPC
> > +(Since Linux 4.11)
> > +The faulting process has exited at the time of
> > +.I UFFDIO_COPY
> > +operation.
> > 
> > To change it now, we would need to involve manpage and other code
> > changes.
> 
> Well, ESRCH is more appropriate so I would rather change it sooner than
> later. But if we are going to risk user space breakage then this is not
> worth the risk. I expected there are very few users of this API
> currently so maybe it won't be a big disaster?

I surely can take care of CRIU, but I don't know if QEMU or certain
database application that uses userfaultfd rely on this API, not mentioning
there maybe other unknown users.

Andrea, what do you think?

> Anyway, at least this is documented so I will leave the decision to you.
> -- 
> Michal Hocko
> SUSE Labs
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

-- 
Sincerely yours,
Mike.

[toc] | [prev] | [next] | [standalone]

#1702112

* Mike Rapoport (rppt@linux.vnet.ibm.com) wrote:
> On Mon, Jul 31, 2017 at 03:45:08PM +0200, Michal Hocko wrote:
> > On Mon 31-07-17 15:32:47, Andrea Arcangeli wrote:
> > > On Mon, Jul 31, 2017 at 02:22:04PM +0200, Michal Hocko wrote:
> > > > On Thu 27-07-17 09:26:59, Mike Rapoport wrote:
> > > > > In the non-cooperative userfaultfd case, the process exit may race with
> > > > > outstanding mcopy_atomic called by the uffd monitor.  Returning -ENOSPC
> > > > > instead of -EINVAL when mm is already gone will allow uffd monitor to
> > > > > distinguish this case from other error conditions.
> > > > 
> > > > Normally we tend to return ESRCH in such case. ENOSPC sounds rather
> > > > confusing...
> > > 
> > > This is in sync and consistent with the retval for UFFDIO_COPY upstream:
> > > 
> > > 	if (mmget_not_zero(ctx->mm)) {
> > > 		ret = mcopy_atomic(ctx->mm, uffdio_copy.dst, uffdio_copy.src,
> > > 				   uffdio_copy.len);
> > > 		mmput(ctx->mm);
> > > 	} else {
> > > 		return -ENOSPC;
> > > 	}
> > > 
> > > If you preferred ESRCH I certainly wouldn't have been against, but we
> > > should have discussed it before it was upstream. All it matters is
> > > it's documented in the great manpage that was written for it as quoted
> > > below.
> > 
> > OK, I wasn't aware of this.
> > 
> > > +.TP
> > > +.B ENOENT
> > > +(Since Linux 4.11)
> > > +The faulting process has changed
> > > +its virtual memory layout simultaneously with outstanding
> > > +.I UFFDIO_COPY
> > > +operation.
> > > +.TP
> > > +.B ENOSPC
> > > +(Since Linux 4.11)
> > > +The faulting process has exited at the time of
> > > +.I UFFDIO_COPY
> > > +operation.
> > > 
> > > To change it now, we would need to involve manpage and other code
> > > changes.
> > 
> > Well, ESRCH is more appropriate so I would rather change it sooner than
> > later. But if we are going to risk user space breakage then this is not
> > worth the risk. I expected there are very few users of this API
> > currently so maybe it won't be a big disaster?
> 
> I surely can take care of CRIU, but I don't know if QEMU or certain
> database application that uses userfaultfd rely on this API, not mentioning
> there maybe other unknown users.
> 
> Andrea, what do you think?

QEMU doesn't care about the errno value, it just reports it.

Dave

> > Anyway, at least this is documented so I will leave the decision to you.
> > -- 
> > Michal Hocko
> > SUSE Labs
> > 
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> > 
> 
> -- 
> Sincerely yours,
> Mike.
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[toc] | [prev] | [next] | [standalone]

#1702208

On Wed, Aug 02, 2017 at 03:34:41PM +0300, Mike Rapoport wrote:
> I surely can take care of CRIU, but I don't know if QEMU or certain
> database application that uses userfaultfd rely on this API, not mentioning
> there maybe other unknown users.
> 
> Andrea, what do you think?

The manpage would need updates, from v4.11 to v4.13 -ENOSPC, from v4.1
-ESRCH and I don't see the benefit and it just looks confusion for
nothing, but if somebody feel strongly about it and does the work (and
risks to take the blame if something breaks...) I wouldn't be against
it, it won't make much of a difference anyway.

The reason I don't see any benefit in code readability is that I don't
see ESRCH as an obviously better retval, because if you grep for ESRCH
you'll see it's a failure to find a process with a certain pid, it is
an obvious retval when you're dealing with processes and pids, but we
never search pids and in fact the pid and the process may be already
gone but we still won't return ESRCH. UFFDIO_COPY never takes a pid as
parameter anywhere so why to return ESRCH? ENOSPC shall be interpreted
"no memory avail to copy anything", ESRCH as far as I can tell, could
be as unexpected as ENOSPC is you don't specify a pid as parameter to
the kernel.

If the mm_users is already zero and the mm is gone it means the
process is gone too, that is true, but the process could be gone
already and we could still obtain the mm_users and run UFFDIO_COPY if
there's async I/O pending or something. There's no association between
process/pid being still alive and the need to run a UFFDIO_COPY and
succeed at it.

Not ever dealing with pids and processes is why not even ESRCH is an
obvious perfect match for such an error, and this is why I think such
a change now would add no tangible pros and only short term cons.

Thanks,
Andrea

[toc] | [prev] | [next] | [standalone]

#1702249

On Wed 02-08-17 17:55:22, Andrea Arcangeli wrote:
> On Wed, Aug 02, 2017 at 03:34:41PM +0300, Mike Rapoport wrote:
> > I surely can take care of CRIU, but I don't know if QEMU or certain
> > database application that uses userfaultfd rely on this API, not mentioning
> > there maybe other unknown users.
> > 
> > Andrea, what do you think?
> 
> The manpage would need updates, from v4.11 to v4.13 -ENOSPC, from v4.1
> -ESRCH and I don't see the benefit and it just looks confusion for
> nothing, but if somebody feel strongly about it and does the work (and
> risks to take the blame if something breaks...) I wouldn't be against
> it, it won't make much of a difference anyway.
> 
> The reason I don't see any benefit in code readability is that I don't
> see ESRCH as an obviously better retval, because if you grep for ESRCH
> you'll see it's a failure to find a process with a certain pid, it is
> an obvious retval when you're dealing with processes and pids, but we
> never search pids and in fact the pid and the process may be already
> gone but we still won't return ESRCH. UFFDIO_COPY never takes a pid as
> parameter anywhere so why to return ESRCH?

ESRCH refers to "no such process". Strictly speaking userfaultfd code is
about a mm which is gone but that is a mere detail. In fact the owner of
the mm is gone as well. You might not refer to the process by its pid
but you are surely refer to a process via its address space. That's why
I think this error code is more appropriate.

But as I've said, this might be really risky to change. My impression
was that userfaultfd is not widely used yet and those can be fixed
easily but if that is not the case then we have to live with the current
ENOSPC.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1702262

On Wed, Aug 02, 2017 at 06:22:49PM +0200, Michal Hocko wrote:
> ESRCH refers to "no such process". Strictly speaking userfaultfd code is
> about a mm which is gone but that is a mere detail. In fact the owner of

Well this whole issue about which retval, is about a mere detail in
the first place, so I don't think you can discount all other mere
details as irrelevant in the evaluation of a change to solve a mere
detail.

> But as I've said, this might be really risky to change. My impression
> was that userfaultfd is not widely used yet and those can be fixed
> easily but if that is not the case then we have to live with the current
> ENOSPC.

The only change would be for userfaultfd non cooperative mode, and
CRIU is the main user of that. So I think it is up to Mike to decide,
I'm fine either ways. I certainly agree ESRCH could be a slightly
better fit, I only wanted to clarify it's not a 100% match either.

[toc] | [prev] | [next] | [standalone]

#1703299

On Wed, Aug 02, 2017 at 06:40:01PM +0200, Andrea Arcangeli wrote:
> On Wed, Aug 02, 2017 at 06:22:49PM +0200, Michal Hocko wrote:
> > ESRCH refers to "no such process". Strictly speaking userfaultfd code is
> > about a mm which is gone but that is a mere detail. In fact the owner of
> 
> Well this whole issue about which retval, is about a mere detail in
> the first place, so I don't think you can discount all other mere
> details as irrelevant in the evaluation of a change to solve a mere
> detail.
> 
> > But as I've said, this might be really risky to change. My impression
> > was that userfaultfd is not widely used yet and those can be fixed
> > easily but if that is not the case then we have to live with the current
> > ENOSPC.
> 
> The only change would be for userfaultfd non cooperative mode, and
> CRIU is the main user of that. So I think it is up to Mike to decide,
> I'm fine either ways. I certainly agree ESRCH could be a slightly
> better fit, I only wanted to clarify it's not a 100% match either.

I'm Ok with updating the code and the man page as long as Michal takes the
blame if anything but CRIU breaks :)

Now, seriously, I believe there are not many users of non-cooperative uffd
if at all and it is very unlikely anybody has it in production.

I'll send a patch with s/ENOSPC/ESRCH in the next few days.

-- 
Sincerely yours,
Mike.

[toc] | [prev] | [next] | [standalone]

#1703434

On Thu, Aug 03, 2017 at 08:24:43PM +0300, Mike Rapoport wrote:
> Now, seriously, I believe there are not many users of non-cooperative uffd
> if at all and it is very unlikely anybody has it in production.
> 
> I'll send a patch with s/ENOSPC/ESRCH in the next few days.

Ok.

Some more thought on this one, enterprise kernels have been shipped
matching the v4.11-v4.12 upstream kernel ABI and I've no time machine
to alter the kABI on those installs.

If you go ahead with the change, the safest would be that you keep
handling -ENOSPC and -ESRCH equally in CRIU code, so there will be no
risk of regression in the short term if somebody is playing with an
upstream CRIU. The alternative would be add uname -r knowledge.

Once it's upstream, I can fixup so further kernel updates will go in
sync. I obviously can't make changes that affects the kABI until it's
upstream and shipped in a official release so things will be out of
sync for a while (and the risk of somebody using ancient kernels will
persist for the mid term).

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web