On 2025-03-04 2:40, Zhu Lingshan wrote:
> On 3/4/2025 1:49 PM, Felix Kuehling wrote:
>> On 2025-02-21 4:23, Zhu Lingshan wrote:
>>> This commit initialized svm lists at where they are
>>> defined. This is defensive programing for security
>>> and consistency.
>>>
>>> Initalizing variables ensures that their states are
>>> always valid, avoiding issues caused by
>>> uninitialized variables that could lead to
>>> unpredictable behaviros.
>> The lists are clearly documented as output parameters in the svm_range_add function definition. I think it's more readable to do the list initialization in svm_range_add to keep the logic of the caller more readable. One suggestion inline that would move the initialization to the caller without cluttering the calling function's code.
>>
>>
>>> And we should not assume the callee would always
>>> initialize them
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@xxxxxxx>
>>> ---
>>> drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 9 +++++----
>>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> index bd3e20d981e0..cbc997449379 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
>>> @@ -2130,11 +2130,7 @@ svm_range_add(struct kfd_process *p, uint64_t start, uint64_t size,
>>>
>>> pr_debug("svms 0x%p [0x%llx 0x%lx]\n", &p->svms, start, last);
>>>
>>> - INIT_LIST_HEAD(update_list);
>>> - INIT_LIST_HEAD(insert_list);
>>> - INIT_LIST_HEAD(remove_list);
>>> INIT_LIST_HEAD(&new_list);
>>> - INIT_LIST_HEAD(remap_list);
>>>
>>> node = interval_tree_iter_first(&svms->objects, start, last);
>>> while (node) {
>>> @@ -3635,6 +3631,11 @@ svm_range_set_attr(struct kfd_process *p, struct mm_struct *mm,
>>> if (r)
>>> return r;
>>>
>>> + INIT_LIST_HEAD(&update_list);
>>> + INIT_LIST_HEAD(&insert_list);
>>> + INIT_LIST_HEAD(&remove_list);
>>> + INIT_LIST_HEAD(&remap_list);
>>> +
>> You could initialize these where they are defined by replacing the struct list_head ... definitions with
>>
>> LIST_HEAD(update_list);
>> LIST_HEAD(insert_list);
> Yes, this is better, I will use LIST_HEAD and remove the initialization in svm_range_add because we don't need to init them twice
>
> By the way, I am not sure the lists are "output parameters", usually an output parameter should carry some information for other functions,
> but here the lists are just defined, even not initialized, and are on the stack. Actually the callee only fills the lists and the caller itself use
> the lists. I suggest to remove the "output parameters" in the code comments.
Input parameters send information from the caller to the called function. Output parameters return information to the caller. In this case the lists are that returned information. Therefore I think calling the list heads output parameters is perfectly valid.
In fact, I could turn your argument on its head: If you move the initialization to the caller, you now rely on that initialization in the callee, which may be a problem if the caller forgets to initialize the list. In then end, it's just a matter of calling conventions. It doesn't really matter where the list heads are initialized as long as they are initialized in one place. In this case the function in question is a static function that's used exactly once. So there is really no point overthinking it.
I'll give you two options: Leave the code as is, or move the list initialization to the definition as I suggested. I will not accept a change that "fixes" this non-issue at the expense of code readability. I disagree with branding this as a security issue in the commit message.
Regards,
Felix
>
> Thanks
> Lingshan
>> ...
>>
>> Regards,
>> Felix
>>
>>
>>> svms = &p->svms;
>>>
>>> mutex_lock(&process_info->lock);
