linux-kernel - Re: [RFC v2 4/5] mm/migrate: Create move_phys

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 3 Oct 2023 13:58:12 -0400
From:   Gregory Price <gregory.price@...verge.com>
To:     Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc:     Gregory Price <gourry.memverge@...il.com>,
        linux-mm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
        linux-cxl@...r.kernel.org, luto@...nel.org, tglx@...utronix.de,
        mingo@...hat.com, bp@...en8.de, dave.hansen@...ux.intel.com,
        hpa@...or.com, arnd@...db.de, akpm@...ux-foundation.org,
        x86@...nel.org
Subject: Re: [RFC v2 4/5] mm/migrate: Create move_phys_pages syscall

On Mon, Oct 02, 2023 at 03:07:46PM +0100, Jonathan Cameron wrote:
> On Tue, 19 Sep 2023 19:09:07 -0400
> Gregory Price <gourry.memverge@...il.com> wrote:
> 
> > @@ -2181,6 +2290,10 @@ static int do_pages_move(struct mm_struct *mm, nodemask_t task_nodes,
> >  	int start, i;
> >  	int err = 0, err1;
> >  
> > +	/* This should never occur in regular operation */
> > +	if (!mm && nodes_weight(task_nodes) > 0)
> 
> You document below that task_nodes isn't used if !mm but it is used
> to indicate this condition...
> 

good point, and in fact the code below does ignore the contents of
nodes_weight if mm is null, so i can just drop this check.

> > +set_status:
> > +		*status = err;
> 
> Given you update it unconditionally this seems to wipe out earlier
> potential errors with the nid.
> 
> > +
> > +		pages++;
> > +		status++;
> > +	}
> > +}
> 

status is a list, the status++ increments the list pointer.

> > +	/*
> > +	 * When the mm argument to do_pages_move is null, the task_nodes
> > +	 * argument is ignored, so pass in an empty nodemask as a dummy.
> 
> If ignored, why bother clearing it?  Looks like you are using it as
> as signal rather than ignoring it. So not sure this comment is correct.
> 
> > +	 */
> > +	nodes_clear(dummy_nodes);

I'm a bit of a stickler around uninitialized memory, seemed better to
ensure the nodelist was empty to force errors associated with an empty
nodelist should future updates break do_pages_move to remove the
ignoring mechanism.

That said, I updated the function such that it does actually ignore it,
but I'll leave the here since there's no harm in clearing the nodemask
and leads to more predictable behavior.

Unless there is particularly strong feelings about this, then i'm fine
to remove it.