Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:
> On Wed, Dec 9, 2020 at 10:05 AM Eric W. Biederman <ebiederm@xxxxxxxxxxxx> wrote:
>>
>> - struct file * file = xchg(&fdt->fd[i], NULL);
>> + struct file * file = fdt->fd[i];
>> if (file) {
>> + rcu_assign_pointer(fdt->fd[i], NULL);
>
> This makes me nervous. Why did we use to do that xchg() there? That
> has atomicity guarantees that now are gone.
>
> Now, this whole thing should be called for just the last ref of the fd
> table, so presumably that atomicity was never needed in the first
> place. But the fact that we did that very expensive xchg() then makes
> me go "there's some reason for it".
>
> Is this xchg() just bogus historical leftover? It kind of looks that
> way. But maybe that change should be done separately?
Removing the xchg in a separate patch seems reasonable. Just to make
the review easier.
I traced the xchg back to 7cf4dc3c8dbf ("move files_struct-related bits
from kernel/exit.c to fs/file.c") when put_files_struct was introduced.
The xchg did not exist before that change.
There were many other xchgs in the code back then so I suspect was left
over from some way an earlier version of the change worked and simply
was not removed when the patch was updated.
Eric
