Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH] mm/hugetlb: Fix a race between hugetlb sysctl handlers
To:     Andrew Morton <akpm@linux-foundation.org>,
        Muchun Song <songmuchun@bytedance.com>
Cc:     npiggin@suse.de, agl@us.ibm.com, ak@linux.intel.com,
        nacc@us.ibm.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20200822095328.61306-1-songmuchun@bytedance.com>
 <20200824135924.b485e000d358cee817c4f05c@linux-foundation.org>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <79800508-54c9-4cda-02de-29b1a6912e75@oracle.com>
Date:   Mon, 24 Aug 2020 14:19:06 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <20200824135924.b485e000d358cee817c4f05c@linux-foundation.org>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 8/24/20 1:59 PM, Andrew Morton wrote:
> On Sat, 22 Aug 2020 17:53:28 +0800 Muchun Song <songmuchun@bytedance.com> wrote:
> 
>> There is a race between the assignment of `table->data` and write value
>> to the pointer of `table->data` in the __do_proc_doulongvec_minmax().
> 
> Where does __do_proc_doulongvec_minmax() write to table->data?
> 
> I think you're saying that there is a race between the assignment of
> ctl_table->table in hugetlb_sysctl_handler_common() and the assignment
> of the same ctl_table->table in hugetlb_overcommit_handler()?
> 
> Or not, maybe I'm being thick.  Can you please describe the race more
> carefully and completely?
> 

I too am looking at this now and do not completely understand the race.
It could be that:

hugetlb_sysctl_handler_common
...
	table->data = &tmp;

and, do_proc_doulongvec_minmax()
...
	return __do_proc_doulongvec_minmax(table->data, table, write, ...
with __do_proc_doulongvec_minmax(void *data, struct ctl_table *table, ...
...
	i = (unsigned long *) data;
	...
		*i = val;
	
So, __do_proc_doulongvec_minmax can be dereferencing and writing to the pointer
in one thread when hugetlb_sysctl_handler_common is setting it in another?

Another confusing part of the message is the stack trace which includes
...
     ? set_max_huge_pages+0x3da/0x4f0
     ? alloc_pool_huge_page+0x150/0x150

which are 'downstream' from these routines.  I don't understand why these
are in the trace.

If the race is with the pointer set and dereference/write, then this type of
fix is OK.  However, if you really have two 'sysadmin type' global operations
racing then one or both are not going to get what they expected.  Instead of
changing the code to 'handle the race', I think it might be acceptable to just
put a big semaphore around it.
-- 
Mike Kravetz