linux-kernel - Re: [PATCH v2 -next] mm/hugetlb_cgroup: register lockdep key for cftype

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: Wed, 19 Jun 2024 10:16:19 +0800
From: xiujianfeng <xiujianfeng@...wei.com>
To: SeongJae Park <sj@...nel.org>
CC: <akpm@...ux-foundation.org>, <linux-mm@...ck.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 -next] mm/hugetlb_cgroup: register lockdep key for
 cftype



On 2024/6/19 7:36, SeongJae Park wrote:
> Hi Xiu,
> 
> 
> On Tue, 18 Jun 2024 07:19:22 +0000 Xiu Jianfeng <xiujianfeng@...wei.com> wrote:
> 
>> When CONFIG_DEBUG_LOCK_ALLOC is enabled, the following commands can
>> trigger a bug,
>>
>> mount -t cgroup2 none /sys/fs/cgroup
>> cd /sys/fs/cgroup
>> echo "+hugetlb" > cgroup.subtree_control
> 
> [...]
>> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
>> index 2b899c4ae968..4ff238ba1250 100644
>> --- a/mm/hugetlb_cgroup.c
>> +++ b/mm/hugetlb_cgroup.c
>> @@ -836,6 +836,8 @@ hugetlb_cgroup_cfttypes_init(struct hstate *h, struct cftype *cft,
>>  			cft->file_offset = MEMFILE_OFFSET0(offset) +
>>  					   MEMFILE_FIELD_SIZE(offset) * idx;
>>  		}
>> +
>> +		lockdep_register_key(&cft->lockdep_key);
>>  	}
>>  }
> 
> I found the latest mm-unstable tree fails build as below, and 'git bisect'
> points this patch.
> 
>     linux/mm/hugetlb_cgroup.c: In function ‘hugetlb_cgroup_cfttypes_init’:
>     linux/mm/hugetlb_cgroup.c:840:42: error: ‘struct cftype’ has no member named ‘lockdep_key’
>       840 |                 lockdep_register_key(&cft->lockdep_key);
>           |                                          ^~
> 
> Maybe we should take care of CONFIG_DEBUG_LOCK_ALLOC undefined case, like
> below?
> 
> 
> diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> index a45065698419..9747c2e64e95 100644
> --- a/mm/hugetlb_cgroup.c
> +++ b/mm/hugetlb_cgroup.c
> @@ -837,7 +837,9 @@ hugetlb_cgroup_cfttypes_init(struct hstate *h, struct cftype *cft,
>                                            MEMFILE_FIELD_SIZE(offset) * idx;
>                 }
> 
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
>                 lockdep_register_key(&cft->lockdep_key);
> +#endif
>         }
>  }

Hi SeongJae,

Thanks for you review.

I think it's better to remove the #ifdef CONFIG_DEBUG_LOCK_ALLOC from
the struct cftype which guards the cft->lockdep_key, because when
CONFIG_DEBUG_LOCK_ALLOC is not defined, struct lock_class_key is an
empty definition which takes no space and can be unconditionally used
within the structure, this may make the code more clean.

To Andrew,

Would you please drop the v2 and pick the v3? thanks.

> 
> [...]
> 
> 
> Thanks,
> SJ