Re: [PATCH] mm: Check for SIGKILL inside dup_mmap() loop.

[Michal Hocko <mhocko@kernel.org>]

On Tue 03-04-18 04:58:57, Matthew Wilcox wrote:
> On Thu, Mar 29, 2018 at 02:30:03PM -0700, Andrew Morton wrote:
> > > @@ -440,6 +440,10 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> > >  			continue;
> > >  		}
> > >  		charge = 0;
> > > +		if (fatal_signal_pending(current)) {
> > > +			retval = -EINTR;
> > > +			goto out;
> > > +		}
> > >  		if (mpnt->vm_flags & VM_ACCOUNT) {
> > >  			unsigned long len = vma_pages(mpnt);
> > 
> > I think a comment explaining why we're doing this would help.
> > 
> > Better would be to add a new function "current_is_oom_killed()" or
> > such, which becomes self-documenting.  Because there are other reasons
> > why a task may have a fatal signal pending.
> 
> I disagree that we need a comment here, or to create an alias.  Someone
> who knows nothing of the oom-killer (like, er, me) reading that code sees
> "Oh, we're checking for fatal signals here.  I guess it doesn't make sense
> to continue forking a process if it's already received a fatal signal."
> 
> One might speculate about the causes of the fatal signal having been
> received and settle on reasons which make sense even without thinking
> of the OOM case.  Because it's why it was introduced, I always think
> about a task blocked on a dead NFS mount.  If it's multithreaded and
> one of the threads called fork() while another thread was blocked on a
> page fault and the dup_mmap() had to wait for the page fault to finish
> ... that would make some kind of sense.

I completely agree. If the check is really correct then it should be
pretty much self explanatory like many other checks. There is absolutely
zero oom specific in there. If a check _is_ oom specific then there is
something fishy going on.
-- 
Michal Hocko
SUSE Labs