Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <152cb376-3793-0dd3-7d2d-d6197b8e014f@oracle.com>
Date:   Fri, 29 Apr 2022 13:59:33 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v1] hugetlbfs: fix hugetlbfs_statfs() locking
Content-Language: en-US
To:     Andrew Morton <akpm@linux-foundation.org>,
        Mina Almasry <almasrymina@google.com>
Cc:     Oscar Salvador <osalvador@suse.de>,
        Muchun Song <songmuchun@bytedance.com>,
        Michal Hocko <mhocko@suse.com>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
References: <20220429202207.3045-1-almasrymina@google.com>
 <20220429133345.d79af45fb107340c31655c8e@linux-foundation.org>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <20220429133345.d79af45fb107340c31655c8e@linux-foundation.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?U2c5b05aQ2F5RnFVVGJKUWFSN2NWZVRZYk1zYmRpYllySnNRQ2lBa245ajlI?=
 =?utf-8?B?M3libkJJL2xVdHZqSnRLSXJ2azY2K3NWTzZ0bHdCaWlnZFUrSFpYR0hVNlZW?=
 =?utf-8?B?UDhNT01oK3UyRUs0U09CUzUxMmk3L2t5ZHdIRmFlZWs5MEFMZy9mMXI1YTUv?=
 =?utf-8?B?YXJLSXBMbnNNMzdaUFpoRE1aQ3pDZU5KV0thMjdYSnhON1Bsd1lhQmJqWUR5?=
 =?utf-8?B?YjB2WGFtaFdiemYxNFl6Z0dsTTBvMy9WRDBvc094L0xDU09mdGNOeGd5eGsy?=
 =?utf-8?B?NDZ3dUgyeGJtN2V5eUM5Zm1PZ0RXKzk4NDM3UzRWcmNpeTJ2cmdSR2x0ZVIz?=
 =?utf-8?B?U1U0czVtQVZqci9LT01kbGQySllGTjlWNHBhVm1pVXRXQUVwcUdPMkNqQ0FV?=
 =?utf-8?B?WEVTNFBXRGhPVTZ3Ymo4aGlteXk5WGZJUXRMaG9aaEVEWVExUkpvaEVtMEdN?=
 =?utf-8?B?a3FERW42NkRTbEIzSU9aWlNuMW1oWUMrem45bDQyelhrMlF6eTZtOFlrVUlp?=
 =?utf-8?B?SEdkZHd5RlF4em9wSTNkbTVWVDY1ZXcyUEhNL21lazBYWTAyMGFwNzZKQlhh?=
 =?utf-8?B?VElvRnB1ZjNSQyt2MGlpWlpuR0xUcE9LU0pXZjgxWXdkS2hSYjZPUmJBR1Bm?=
 =?utf-8?B?MktLK2dxWVphU1BlU0hFTkFueWJjZGFJdEVYY2FUWG5RcWxvc080MXBMTGZ0?=
 =?utf-8?B?YkRVQ3lHMld1empCRzBQRlBVck14TlBUeG0wYTRJWG5hcjhsaklESmpOdFh2?=
 =?utf-8?B?by9iL1ZIMU1RT1dFcjlkcEhMd3Z6aEFmbE02NjhaTGM5SHdKZjRJczN6S0hm?=
 =?utf-8?B?dzY0RFBETmhlMWRmbmF3K1RyWFlTSWxQNDlXSUdRQ09VTUVzblcycERQNW1B?=
 =?utf-8?B?YWFvT3lzVHNPOHNBeG9jOUNsb09WemJLL2NyY1BTblRqZi80aVN1ZDhBeW16?=
 =?utf-8?B?czRDcm5GYUtXdzVvMEI0d01GMG5nMDNtb2Z2dWJrRzd4SzdmL01Zd1h4TmVn?=
 =?utf-8?B?cEQxK2hKY2E1NVZjU1kxNjFVWXQ3VGRjWnNSbXkyYTM3TjFmL1BkS3UzWDgz?=
 =?utf-8?B?aW5KWUZ3NUFEOFNXek1iZjRSdFhoMTkvZENhN3J4Q1l6Um91b3pvaFp5MmlR?=
 =?utf-8?B?RTJyQ3pGOFArWEMyR2VUVC81TXVNbE5xTWdjZWNKbUdjK3dQNXJPWXU1amxT?=
 =?utf-8?B?QjJnRGROSnYxanA0U2FFNWc2VG4yR0ZtNU4vOEtMY3RjY0FGZjh1NGxkR2pV?=
 =?utf-8?B?dSsvRzU5ZTRNa3FQMnFiV0FhWm9HUExyL2JlTFYyczVoeTFpMmQ1Z1NyaCtE?=
 =?utf-8?B?K1pQZTV6UWJDZFQ2OFk3OW5uU2UwejVHVnhkN08xczUwUTRtRVdpVzF2L0ZO?=
 =?utf-8?B?ckE3R1A1ejZGR3A3YU56b0lVVTRLK3p0cWpyRW1EV3JlMjVzeE10aHplOTM3?=
 =?utf-8?B?THE0Y0J1NnlTMk5EM1pJbEIwdXg0TEpYWktqeUovdDJFYzUvOXJFT3VtVzF1?=
 =?utf-8?B?WnZzcVhFcWhLZzFpWWc3VHZ0dDJSSjc1NGtZSzdXc2tyVzEvQW9jclhkUkJU?=
 =?utf-8?B?RzJhTlNKUzN1dUZpS1d5aTRsUDlqejZ2SnBpMzV4OXN5RTdTMGdPMUlNd2V2?=
 =?utf-8?B?VkRPanQ3RzFPbWwreTJEUlVqS1VkM1RjbkxjTXN6YkpRSmVGNWViMW9XUC81?=
 =?utf-8?B?NjZGaW81dFAzY3FHRkFYY3RIWXg4YTA4M1BRcW1ncVNDa3BIZE5zdUtKRy9o?=
 =?utf-8?B?S0JPV1lVR0JzbktiZzBvaWJJaGxCbU52WWdnaHJ6cXB1MDNYMDdwcmNrblJ2?=
 =?utf-8?B?djJuVDA3V0ovdnVZMFp1azcvTENpYTErNHZGWDNCZFFGcmFBaXBQMEJZNUFR?=
 =?utf-8?B?NFlBeHFpVmtWSEJ6TWdYeVczM1VZaG1ESmhvMk5aeC9jazRSSlB5Zm1VRHpq?=
 =?utf-8?B?TkhNNmNlUEFLUitjTXk1ZVJ3Z21BMy9ISTM1RHVFRE9MWk5qTERkVmthWHJp?=
 =?utf-8?B?ZkZqR0hkKzBTb212NlBEYjFkT085dVZYeTYrUndWRUJ2YTFOcTQ4dHdLdlVV?=
 =?utf-8?B?MFNsb2o5UmYzd3pvdVpHaklISkpNK1BzbkFMdCtjbGJOVDJFTmZSZ0ZFOHpW?=
 =?utf-8?B?cC9hbEhlbnNIMURzbFJpREJtdVBPUHpvK1czU0lwVHYyZnJGdSt0UklPSnl1?=
 =?utf-8?B?RnMyb0NaVG1rVys0TDFqaW9aS0pldFZrTHFaaHcwSDBGSlZaVFphOFJ3UHox?=
 =?utf-8?B?Y2QxSXAwaWVsV1g1MGkwaUtGM1hsbytMNmM1SXpZSy9iZDBWaHdRRGRZSC9G?=
 =?utf-8?B?cjhZZnR3dTcyb1R4alNMQUltMGFDMjlYU25NM3ZiaHVEZUI0a2tKN2UwL1N5?=
 =?utf-8?Q?tTgGtT8o0s6/vdH4=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 32190f39-a1f0-4b1a-0533-08da2a232b17
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Apr 2022 20:59:36.4465
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: eUi7/YDmb8rGm6i4is4FJYEDjWGFAXYI9ThUYPqufmPg3MEObwxgSx78mWPmR7jPCDRpSmpbq1dStmQ8bxfxpg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3985
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-29_07:2022-04-28,2022-04-29 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 spamscore=0 adultscore=0
 mlxscore=0 bulkscore=0 suspectscore=0 malwarescore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204290113
X-Proofpoint-ORIG-GUID: nOlo8qVaYjsuL2JqDnsDfvsNtGKB0yrV
X-Proofpoint-GUID: nOlo8qVaYjsuL2JqDnsDfvsNtGKB0yrV
X-Spam-Status: No, score=-3.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 4/29/22 13:33, Andrew Morton wrote:
> On Fri, 29 Apr 2022 13:22:06 -0700 Mina Almasry <almasrymina@google.com> wrote:
> 
>> After commit db71ef79b59b ("hugetlb: make free_huge_page irq safe"),
>> the subpool lock should be locked with spin_lock_irq() and all call
>> sites was modified as such, except for the ones in hugetlbfs_statfs().
>>
>> ...
>>
>> --- a/fs/hugetlbfs/inode.c
>> +++ b/fs/hugetlbfs/inode.c
>> @@ -1048,12 +1048,12 @@ static int hugetlbfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>  		if (sbinfo->spool) {
>>  			long free_pages;
>>
>> -			spin_lock(&sbinfo->spool->lock);
>> +			spin_lock_irq(&sbinfo->spool->lock);
>>  			buf->f_blocks = sbinfo->spool->max_hpages;
>>  			free_pages = sbinfo->spool->max_hpages
>>  				- sbinfo->spool->used_hpages;
>>  			buf->f_bavail = buf->f_bfree = free_pages;
>> -			spin_unlock(&sbinfo->spool->lock);
>> +			spin_unlock_irq(&sbinfo->spool->lock);
>>  			buf->f_files = sbinfo->max_inodes;
>>  			buf->f_ffree = sbinfo->free_inodes;
>>  		}
> 
> Looks good.

Agree, thanks Mina!
Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>

> 
> This seems to be theoretically deadlockable and less theoretically
> lockdep splattable, so I'm inclined to cc:stable on this.
> 
> I wonder why we didn't do that with db71ef79b59bb2e78dc4.
> 

I do not think it was considered because the "less theoretically lockdep splattable" was so rare.

IIRC, the issue of possibly freeing hugetlb pages in IRQ context existed
from almost the beginning of hugetlb.  It was first discovered and 'addressed'
with c77c0a8ac4c5.  That was not cc:stable.  Then it was discovered that c77c0a8ac4c5 was not complete, so db71ef79b59b effectively replaced c77c0a8ac4c5.  That also was not cc:stable.  I guess we could cc:stable this.

Mina, did you find this with lockdep or just code inspection?
-- 
Mike Kravetz