linux-kernel - Re: [PATCH 3/4] block: add IOC_PR_READ

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <fadbd728-6810-49de-905d-214c2f72a857@kernel.org>
Date: Mon, 1 Dec 2025 17:26:27 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Stefan Hajnoczi <stefanha@...hat.com>
Cc: Hannes Reinecke <hare@...e.de>, linux-block@...r.kernel.org,
 "Martin K. Petersen" <martin.petersen@...cle.com>,
 linux-kernel@...r.kernel.org,
 "James E.J. Bottomley" <James.Bottomley@...senpartnership.com>,
 Mike Christie <michael.christie@...cle.com>, Jens Axboe <axboe@...nel.dk>,
 linux-nvme@...ts.infradead.org, Keith Busch <kbusch@...nel.org>,
 Sagi Grimberg <sagi@...mberg.me>, linux-scsi@...r.kernel.org,
 Christoph Hellwig <hch@....de>
Subject: Re: [PATCH 3/4] block: add IOC_PR_READ_KEYS ioctl

On 01/12/2025 16:06, Stefan Hajnoczi wrote:
> On Sat, Nov 29, 2025 at 03:32:35PM +0100, Krzysztof Kozlowski wrote:
>> On 27/11/2025 08:07, Hannes Reinecke wrote:
>>>
>>>> +	size_t keys_info_len = struct_size(keys_info, keys, inout.num_keys);
>>>> +
>>>> +	keys_info = kzalloc(keys_info_len, GFP_KERNEL);
>>>> +	if (!keys_info)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	keys_info->num_keys = inout.num_keys;
>>>> +
>>>> +	ret = ops->pr_read_keys(bdev, keys_info);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	/* Copy out individual keys */
>>>> +	u64 __user *keys_ptr = u64_to_user_ptr(inout.keys_ptr);
>>>> +	u32 num_copy_keys = min(inout.num_keys, keys_info->num_keys);
>>>> +	size_t keys_copy_len = num_copy_keys * sizeof(keys_info->keys[0]);
>>>
>>> We just had the discussion about variable declarations on the ksummit 
>>> lists; I really would prefer to have all declarations at the start of 
>>> the scope (read: at the start of the function here).
>>
>> Then also cleanup.h should not be used here.
> 
> Hi Krzysztof,
> The documentation in cleanup.h says:
> 
>  * Given that the "__free(...) = NULL" pattern for variables defined at
>  * the top of the function poses this potential interdependency problem
>  * the recommendation is to always define and assign variables in one
>        ^^^^^^^^^^^^^^
>  * statement and not group variable definitions at the top of the
>  * function when __free() is used.
> 
> This is a recommendation, not mandatory. It is also describing a
> scenario that does not apply here.

If you have actual argument, so allocation in some if branch, the of course.

> 
> There are many examples of existing users of __free() initialized to
> NULL:
> 
>   $ git grep '__free(' | grep ' = NULL' | wc -l
>   491
> 

There is tons of incorrect usage, some I started already fixing.
Maintainers were changing when applying the correct code (correct patch)
into incorrect declaration without constructor and separate assignment.

Then contributors started adding patches at least making NULL
assignment, but still not following recommendation.

Then contributors started adding patches mixing cleanup.h with gotos,
also clearly discouraged.

Sorry, but please never use that argument. People did not read cleanup.h
and produced poor code. Maintainers did not read cleanup.h and changed
correct code into poor code.

Existing poor code, just its existence, is never the actual argument in
discussion.

> To me it seems like it is okay to use cleanup.h in this fashion. Did I
> miss something?

Best regards,
Krzysztof