linux-kernel - Re: [PATCH v5 1/3] fs/file.c: remove sanity_check and add likely/unlikely in alloc

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d5c8edc6-68fc-44fd-90c6-5deb7c027566@intel.com>
Date: Thu, 15 Aug 2024 10:49:40 +0800
From: "Ma, Yu" <yu.ma@...el.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: brauner@...nel.org, jack@...e.cz, mjguzik@...il.com, edumazet@...gle.com,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
 pan.deng@...el.com, tianyou.li@...el.com, tim.c.chen@...el.com,
 tim.c.chen@...ux.intel.com, yu.ma@...el.com
Subject: Re: [PATCH v5 1/3] fs/file.c: remove sanity_check and add
 likely/unlikely in alloc_fd()


On 8/15/2024 5:38 AM, Al Viro wrote:
> On Wed, Jul 17, 2024 at 10:50:16AM -0400, Yu Ma wrote:
>> alloc_fd() has a sanity check inside to make sure the struct file mapping to the
>> allocated fd is NULL. Remove this sanity check since it can be assured by
>> exisitng zero initilization and NULL set when recycling fd. Meanwhile, add
>> likely/unlikely and expand_file() call avoidance to reduce the work under
>> file_lock.
>> +	if (unlikely(fd >= fdt->max_fds)) {
>> +		error = expand_files(files, fd);
>> +		if (error < 0)
>> +			goto out;
>>   
>> -	/*
>> -	 * If we needed to expand the fs array we
>> -	 * might have blocked - try again.
>> -	 */
>> -	if (error)
>> -		goto repeat;
>> +		/*
>> +		 * If we needed to expand the fs array we
>> +		 * might have blocked - try again.
>> +		 */
>> +		if (error)
>> +			goto repeat;
> With that change you can't get 0 from expand_files() here, so the
> last goto should be unconditional.  The only case when expand_files()
> returns 0 is when it has found the descriptor already being covered
> by fdt; since fdt->max_fds is stabilized by ->files_lock we are
> holding here, comparison in expand_files() will give the same
> result as it just had.
>
> IOW, that goto repeat should be unconditional.  The fun part here is
> that this was the only caller that distinguished between 0 and 1...

Yes, thanks Al, fully agree with you. The if (error) could be removed 
here as the above unlikely would make sure no 0 return here. Should I 
submit another version of patch set to update it, or you may help to 
update it directly during merge? I'm not very familiar with the rules, 
please let me know if I'd update and I'll take action soon. Thanks again 
for your careful check here.