fs/smb/server/transport_tcp.c | 22 ++-------------------- 1 file changed, 2 insertions(+), 20 deletions(-)
The ksmbd listener thread was using busy waiting on a listening socket by
calling kernel_accept() with SOCK_NONBLOCK and retrying every 100ms on
-EAGAIN. Since this thread is dedicated to accepting new connections,
there is no need for non-blocking mode.
Switch to a blocking accept() call instead, allowing the thread to sleep
until a new connection arrives. This avoids unnecessary wakeups and CPU
usage.
Also remove:
- TCP_NODELAY, which has no effect on a listening socket.
- sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
to return -EAGAIN prematurely.
Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
Signed-off-by: Qingfang Deng <dqfext@gmail.com>
---
fs/smb/server/transport_tcp.c | 22 ++--------------------
1 file changed, 2 insertions(+), 20 deletions(-)
diff --git a/fs/smb/server/transport_tcp.c b/fs/smb/server/transport_tcp.c
index 7a1e3dcc2cde..57a6aa98e7de 100644
--- a/fs/smb/server/transport_tcp.c
+++ b/fs/smb/server/transport_tcp.c
@@ -46,11 +46,6 @@ static struct interface *alloc_iface(char *ifname);
#define TCP_TRANS(t) ((struct tcp_transport *)container_of(t, \
struct tcp_transport, transport))
-static inline void ksmbd_tcp_nodelay(struct socket *sock)
-{
- tcp_sock_set_nodelay(sock->sk);
-}
-
static inline void ksmbd_tcp_reuseaddr(struct socket *sock)
{
sock_set_reuseaddr(sock->sk);
@@ -241,15 +236,10 @@ static int ksmbd_kthread_fn(void *p)
mutex_unlock(&iface->sock_release_lock);
break;
}
- ret = kernel_accept(iface->ksmbd_socket, &client_sk,
- SOCK_NONBLOCK);
+ ret = kernel_accept(iface->ksmbd_socket, &client_sk, 0);
mutex_unlock(&iface->sock_release_lock);
- if (ret) {
- if (ret == -EAGAIN)
- /* check for new connections every 100 msecs */
- schedule_timeout_interruptible(HZ / 10);
+ if (ret)
continue;
- }
if (!server_conf.max_ip_connections)
goto skip_max_ip_conns_limit;
@@ -455,10 +445,6 @@ static void tcp_destroy_socket(struct socket *ksmbd_socket)
if (!ksmbd_socket)
return;
- /* set zero to timeout */
- ksmbd_tcp_rcv_timeout(ksmbd_socket, 0);
- ksmbd_tcp_snd_timeout(ksmbd_socket, 0);
-
ret = kernel_sock_shutdown(ksmbd_socket, SHUT_RDWR);
if (ret)
pr_err("Failed to shutdown socket: %d\n", ret);
@@ -505,7 +491,6 @@ static int create_socket(struct interface *iface)
release_sock(ksmbd_socket->sk);
}
- ksmbd_tcp_nodelay(ksmbd_socket);
ksmbd_tcp_reuseaddr(ksmbd_socket);
ret = sock_setsockopt(ksmbd_socket,
@@ -529,9 +514,6 @@ static int create_socket(struct interface *iface)
goto out_error;
}
- ksmbd_socket->sk->sk_rcvtimeo = KSMBD_TCP_RECV_TIMEOUT;
- ksmbd_socket->sk->sk_sndtimeo = KSMBD_TCP_SEND_TIMEOUT;
-
ret = kernel_listen(ksmbd_socket, KSMBD_SOCKET_BACKLOG);
if (ret) {
pr_err("Port listen() error: %d\n", ret);
--
2.43.0Am 30.10.25 um 07:47 schrieb Qingfang Deng: > The ksmbd listener thread was using busy waiting on a listening socket by > calling kernel_accept() with SOCK_NONBLOCK and retrying every 100ms on > -EAGAIN. Since this thread is dedicated to accepting new connections, > there is no need for non-blocking mode. > > Switch to a blocking accept() call instead, allowing the thread to sleep > until a new connection arrives. This avoids unnecessary wakeups and CPU > usage. > > Also remove: > - TCP_NODELAY, which has no effect on a listening socket. > - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept() > to return -EAGAIN prematurely. Aren't these inherited to the accepted sockets? So we need to apply them to the accepted sockets now instead of dropping them completely? metze
Am 11.11.25 um 07:55 schrieb Stefan Metzmacher:
> Am 30.10.25 um 07:47 schrieb Qingfang Deng:
>> The ksmbd listener thread was using busy waiting on a listening socket by
>> calling kernel_accept() with SOCK_NONBLOCK and retrying every 100ms on
>> -EAGAIN. Since this thread is dedicated to accepting new connections,
>> there is no need for non-blocking mode.
>>
>> Switch to a blocking accept() call instead, allowing the thread to sleep
>> until a new connection arrives. This avoids unnecessary wakeups and CPU
>> usage.
>>
>> Also remove:
>> - TCP_NODELAY, which has no effect on a listening socket.
>> - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
>> to return -EAGAIN prematurely.
>
> Aren't these inherited to the accepted sockets?
> So we need to apply them to the accepted sockets now
> instead of dropping them completely?
Actually the timeouts are added to the client connection,
but not the TCP_NODELAY.
But looking at it more detailed I'm wondering if this might
introduce a deadlock.
We have this in the accepting thread:
while (!kthread_should_stop()) {
mutex_lock(&iface->sock_release_lock);
if (!iface->ksmbd_socket) {
mutex_unlock(&iface->sock_release_lock);
break;
}
ret = kernel_accept(iface->ksmbd_socket, &client_sk, 0);
mutex_unlock(&iface->sock_release_lock);
if (ret)
continue;
And in the stopping code this:
case NETDEV_DOWN:
iface = ksmbd_find_netdev_name_iface_list(netdev->name);
if (iface && iface->state == IFACE_STATE_CONFIGURED) {
ksmbd_debug(CONN, "netdev-down event: netdev(%s) is going down\n",
iface->name);
tcp_stop_kthread(iface->ksmbd_kthread);
iface->ksmbd_kthread = NULL;
mutex_lock(&iface->sock_release_lock);
tcp_destroy_socket(iface->ksmbd_socket);
iface->ksmbd_socket = NULL;
mutex_unlock(&iface->sock_release_lock);
iface->state = IFACE_STATE_DOWN;
break;
}
I guess that now kernel_accept() call waits forever holding iface->sock_release_lock
and tcp_stop_kthread(iface->ksmbd_kthread); doesn't have any impact anymore
as we may never reach kthread_should_stop() anymore.
We may want to do a kernel_sock_shutdown(ksmbd_socket, SHUT_RDWR) after
tcp_stop_kthread(iface->ksmbd_kthread); but before mutex_lock(&iface->sock_release_lock);
so that kernel_accept() hopefully returns directly.
And we only call sock_release(ksmbd_socket); under the iface->sock_release_lock mutex.
metze
Hi Stefan,
On Tue, Nov 11, 2025 at 3:16 PM Stefan Metzmacher <metze@samba.org> wrote:
> >> Also remove:
> >> - TCP_NODELAY, which has no effect on a listening socket.
> >> - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
> >> to return -EAGAIN prematurely.
> >
> > Aren't these inherited to the accepted sockets?
> > So we need to apply them to the accepted sockets now
> > instead of dropping them completely?
You're right, TCP_NODELAY of a new accepted socket is inherited from
the listen socket, so it should not be removed.
>
> Actually the timeouts are added to the client connection,
> but not the TCP_NODELAY.
>
> But looking at it more detailed I'm wondering if this might
> introduce a deadlock.
>
> We have this in the accepting thread:
>
> while (!kthread_should_stop()) {
> mutex_lock(&iface->sock_release_lock);
> if (!iface->ksmbd_socket) {
> mutex_unlock(&iface->sock_release_lock);
> break;
> }
> ret = kernel_accept(iface->ksmbd_socket, &client_sk, 0);
> mutex_unlock(&iface->sock_release_lock);
> if (ret)
> continue;
>
>
> And in the stopping code this:
>
> case NETDEV_DOWN:
> iface = ksmbd_find_netdev_name_iface_list(netdev->name);
> if (iface && iface->state == IFACE_STATE_CONFIGURED) {
> ksmbd_debug(CONN, "netdev-down event: netdev(%s) is going down\n",
> iface->name);
> tcp_stop_kthread(iface->ksmbd_kthread);
> iface->ksmbd_kthread = NULL;
> mutex_lock(&iface->sock_release_lock);
> tcp_destroy_socket(iface->ksmbd_socket);
> iface->ksmbd_socket = NULL;
> mutex_unlock(&iface->sock_release_lock);
>
> iface->state = IFACE_STATE_DOWN;
> break;
> }
>
>
>
> I guess that now kernel_accept() call waits forever holding iface->sock_release_lock
> and tcp_stop_kthread(iface->ksmbd_kthread); doesn't have any impact anymore
> as we may never reach kthread_should_stop() anymore.
>
> We may want to do a kernel_sock_shutdown(ksmbd_socket, SHUT_RDWR) after
> tcp_stop_kthread(iface->ksmbd_kthread); but before mutex_lock(&iface->sock_release_lock);
> so that kernel_accept() hopefully returns directly.
> And we only call sock_release(ksmbd_socket); under the iface->sock_release_lock mutex.
In kernel v6.1 or later, kthread_stop() in tcp_stop_kthread() will
send a signal to the ksmbd kthread so accept() will return -EINTR.
Before v6.1 it can actually get stuck, as accept() will block forever.
If you're fixing the issue when this patch was backported to versions
before v6.1, this will not work, because kthread_stop() blocks until
the target kthread returns, so shutdown() will never get called. The
sock_release_lock mutex seems redundant because of that.
Instead, shutdown() can be called _before_ kthread_stop() so accept()
will return -EINVAL.
Namjae, should I send a v2 with both issues addressed?
-- Qingfang
On Tue, Nov 11, 2025 at 5:03 PM Qingfang Deng <dqfext@gmail.com> wrote:
>
> Hi Stefan,
>
> On Tue, Nov 11, 2025 at 3:16 PM Stefan Metzmacher <metze@samba.org> wrote:
> > >> Also remove:
> > >> - TCP_NODELAY, which has no effect on a listening socket.
> > >> - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
> > >> to return -EAGAIN prematurely.
> > >
> > > Aren't these inherited to the accepted sockets?
> > > So we need to apply them to the accepted sockets now
> > > instead of dropping them completely?
>
> You're right, TCP_NODELAY of a new accepted socket is inherited from
> the listen socket, so it should not be removed.
>
> >
> > Actually the timeouts are added to the client connection,
> > but not the TCP_NODELAY.
> >
> > But looking at it more detailed I'm wondering if this might
> > introduce a deadlock.
> >
> > We have this in the accepting thread:
> >
> > while (!kthread_should_stop()) {
> > mutex_lock(&iface->sock_release_lock);
> > if (!iface->ksmbd_socket) {
> > mutex_unlock(&iface->sock_release_lock);
> > break;
> > }
> > ret = kernel_accept(iface->ksmbd_socket, &client_sk, 0);
> > mutex_unlock(&iface->sock_release_lock);
> > if (ret)
> > continue;
> >
> >
> > And in the stopping code this:
> >
> > case NETDEV_DOWN:
> > iface = ksmbd_find_netdev_name_iface_list(netdev->name);
> > if (iface && iface->state == IFACE_STATE_CONFIGURED) {
> > ksmbd_debug(CONN, "netdev-down event: netdev(%s) is going down\n",
> > iface->name);
> > tcp_stop_kthread(iface->ksmbd_kthread);
> > iface->ksmbd_kthread = NULL;
> > mutex_lock(&iface->sock_release_lock);
> > tcp_destroy_socket(iface->ksmbd_socket);
> > iface->ksmbd_socket = NULL;
> > mutex_unlock(&iface->sock_release_lock);
> >
> > iface->state = IFACE_STATE_DOWN;
> > break;
> > }
> >
> >
> >
> > I guess that now kernel_accept() call waits forever holding iface->sock_release_lock
> > and tcp_stop_kthread(iface->ksmbd_kthread); doesn't have any impact anymore
> > as we may never reach kthread_should_stop() anymore.
> >
> > We may want to do a kernel_sock_shutdown(ksmbd_socket, SHUT_RDWR) after
> > tcp_stop_kthread(iface->ksmbd_kthread); but before mutex_lock(&iface->sock_release_lock);
> > so that kernel_accept() hopefully returns directly.
> > And we only call sock_release(ksmbd_socket); under the iface->sock_release_lock mutex.
>
> In kernel v6.1 or later, kthread_stop() in tcp_stop_kthread() will
> send a signal to the ksmbd kthread so accept() will return -EINTR.
> Before v6.1 it can actually get stuck, as accept() will block forever.
>
> If you're fixing the issue when this patch was backported to versions
> before v6.1, this will not work, because kthread_stop() blocks until
> the target kthread returns, so shutdown() will never get called. The
> sock_release_lock mutex seems redundant because of that.
> Instead, shutdown() can be called _before_ kthread_stop() so accept()
> will return -EINVAL.
>
> Namjae, should I send a v2 with both issues addressed?
Yes, please send the v2 patch.
Thanks.
>
> -- Qingfang
On Thu, Oct 30, 2025 at 3:47 PM Qingfang Deng <dqfext@gmail.com> wrote:
>
> The ksmbd listener thread was using busy waiting on a listening socket by
> calling kernel_accept() with SOCK_NONBLOCK and retrying every 100ms on
> -EAGAIN. Since this thread is dedicated to accepting new connections,
> there is no need for non-blocking mode.
>
> Switch to a blocking accept() call instead, allowing the thread to sleep
> until a new connection arrives. This avoids unnecessary wakeups and CPU
> usage.
>
> Also remove:
> - TCP_NODELAY, which has no effect on a listening socket.
> - sk_rcvtimeo and sk_sndtimeo assignments, which only caused accept()
> to return -EAGAIN prematurely.
>
> Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> Signed-off-by: Qingfang Deng <dqfext@gmail.com>
Applied it to #ksmbd-for-next-next.
Thanks!
Hi Namjae,
On Thu, Oct 30, 2025 at 4:11 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> > Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> > Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> Applied it to #ksmbd-for-next-next.
> Thanks!
I just found that this depends on another commit which is not in
kernel versions earlier than v6.1:
a7c01fa93aeb ("signal: break out of wait loops on kthread_stop()")
With the current Fixes tag, this commit will be backported to v5.15
automatically. But without said commit, kthread_stop() cannot wake up
a blocking kernel_accept().
Should I change the Fixes tag, or inform linux-stable not to backport
this patch to v5.15?
+Cc: Jason, Sasha, and GregKH
Regards,
Qingfang
On Fri, Oct 31, 2025 at 03:32:06PM +0800, Qingfang Deng wrote:
> Hi Namjae,
>
> On Thu, Oct 30, 2025 at 4:11 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> > > Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> > > Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> > Applied it to #ksmbd-for-next-next.
> > Thanks!
>
> I just found that this depends on another commit which is not in
> kernel versions earlier than v6.1:
> a7c01fa93aeb ("signal: break out of wait loops on kthread_stop()")
>
> With the current Fixes tag, this commit will be backported to v5.15
> automatically. But without said commit, kthread_stop() cannot wake up
> a blocking kernel_accept().
> Should I change the Fixes tag, or inform linux-stable not to backport
> this patch to v5.15?
Email stable@vger.kernel.org when it lands in Linus's tree to not
backport it that far.
thanks,
greg k-h
On Fri, Oct 31, 2025 at 3:44 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Fri, Oct 31, 2025 at 03:32:06PM +0800, Qingfang Deng wrote:
> > Hi Namjae,
> >
> > On Thu, Oct 30, 2025 at 4:11 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> > > > Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> > > > Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> > > Applied it to #ksmbd-for-next-next.
> > > Thanks!
> >
> > I just found that this depends on another commit which is not in
> > kernel versions earlier than v6.1:
> > a7c01fa93aeb ("signal: break out of wait loops on kthread_stop()")
> >
> > With the current Fixes tag, this commit will be backported to v5.15
> > automatically. But without said commit, kthread_stop() cannot wake up
> > a blocking kernel_accept().
> > Should I change the Fixes tag, or inform linux-stable not to backport
> > this patch to v5.15?
>
> Email stable@vger.kernel.org when it lands in Linus's tree to not
> backport it that far.
Noted. Thanks!
On Fri, Oct 31, 2025 at 4:49 PM Qingfang Deng <dqfext@gmail.com> wrote:
>
> On Fri, Oct 31, 2025 at 3:44 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Fri, Oct 31, 2025 at 03:32:06PM +0800, Qingfang Deng wrote:
> > > Hi Namjae,
> > >
> > > On Thu, Oct 30, 2025 at 4:11 PM Namjae Jeon <linkinjeon@kernel.org> wrote:
> > > > > Fixes: 0626e6641f6b ("cifsd: add server handler for central processing and tranport layers")
> > > > > Signed-off-by: Qingfang Deng <dqfext@gmail.com>
> > > > Applied it to #ksmbd-for-next-next.
> > > > Thanks!
> > >
> > > I just found that this depends on another commit which is not in
> > > kernel versions earlier than v6.1:
> > > a7c01fa93aeb ("signal: break out of wait loops on kthread_stop()")
> > >
> > > With the current Fixes tag, this commit will be backported to v5.15
> > > automatically. But without said commit, kthread_stop() cannot wake up
> > > a blocking kernel_accept().
> > > Should I change the Fixes tag, or inform linux-stable not to backport
> > > this patch to v5.15?
> >
> > Email stable@vger.kernel.org when it lands in Linus's tree to not
> > backport it that far.
>
> Noted. Thanks!
I think it would be better to use the stable tag + kernel version tag
instead of the fixes tag.
Cc: stable@vger.kernel.org # v6.1+
Also, I don't think this patch necessarily needs to be merged into the
stable kernels.
Thanks.
© 2016 - 2025 Red Hat, Inc.