On 06/03/2024 19:07, Dmitry Antipov wrote:
On 3/6/24 17:45, Wen Gu wrote:
IIUC, the fallback (or more precisely the private_data change)
essentially
always happens when the lock_sock(smc->sk) is held, except in
smc_listen_work()
or smc_listen_decline(), but at that moment, userspace program can not
yet
acquire this new socket to add fasync entries to the fasync_list.
So IMHO, the above patch should work, since it checks the private_data
under
the lock_sock(sk). But if I missed something, please correct me.
Well, the whole picture is somewhat more complicated. Consider the
following diagram (an underlying kernel socket is in [], e.g. [smc->sk]):
Thread 0 Thread 1
ioctl(sock, FIOASYNC, [1])
...
sock = filp->private_data;
lock_sock(sock [smc->sk]);
sock_fasync(sock, ..., 1) ; new fasync_struct linked to smc->sk
release_sock(sock [smc->sk]);
...
lock_sock([smc->sk]);
...
smc_switch_to_fallback()
...
smc->clcsock->file->private_data =
smc->clcsock;
...
release_sock([smc->sk]);
ioctl(sock, FIOASYNC, [0])
...
sock = filp->private_data;
lock_sock(sock [smc->clcsock]);
sock_fasync(sock, ..., 0) ; nothing to unlink from smc->clcsock
; since fasync entry was linked to smc->sk
release_sock(sock [smc->clcsock]);
...
close(sock [smc->clcsock]);
__fput(...);
file->f_op->fasync(sock, [0]) ;
always failed -
;
should use
;
smc->sk instead
file->f_op->release()
...
smc_restore_fallback_changes()
...
file->private_data = smc->sk.sk_socket;
That is, smc_restore_fallback_changes() restores filp->private_data to
smc->sk. If __fput() would have called file->f_op->release() _before_
file->f_op->fasync(), the fix would be as simple as adding
smc->sk.sk_socket->wq.fasync_list = smc->clcsock->wq.fasync_list;
to smc_restore_fallback_changes(). But since file->f_op->fasync() is called
before file->f_op->release(), the former always makes an attempt to
unlink fasync
entry from smc->clcsock instead of smc->sk, thus introducing the memory
leak.
And an idea with shared wait queue was intended in attempt to eliminate
this chicken-egg lookalike problem completely.
Dmitry
Me and Gerd had another look at this.
The infrastructure for what i proposed in the last E-Mail regarding the
ioctl function handler is already there (af_smc.c#smc_ioctl).
There we already check if we are in a active fallback to send the ioctls
to the clcsock instead of the sk socket.
```
lock_sock(&smc->sk);
if (smc->use_fallback) {
if (!smc->clcsock) {
release_sock(&smc->sk);
return -EBADF;
}
answ = smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
release_sock(&smc->sk);
return answ;
}
```
We think it might be an option to secure the path in this function with
the smc->clcsock_release_lock.
```
lock_sock(&smc->sk);
if (smc->use_fallback) {
if (!smc->clcsock) {
release_sock(&smc->sk);
return -EBADF;
}
+ mutex_lock(&smc->clcsock_release_lock);
answ = smc->clcsock->ops->ioctl(smc->clcsock, cmd, arg);
+ mutex_unlock(&smc->clcsock_release_lock);
release_sock(&smc->sk);
return answ;
}
```
What do yo think about this?
I'm going to test this idea and see if we canget rid of the leak this way.
Thanks
- Jan & Gerd
