Message-ID: <13b10448-9743-4f05-9662-370862c2040e@kzalloc.com>
Date: Tue, 19 Aug 2025 23:56:36 +0900
From: Yunseong Kim <ysk@...lloc.com>
To: Namjae Jeon <linkinjeon@...nel.org>
Cc: Steve French <smfrench@...il.com>, Stefan Metzmacher <metze@...ba.org>,
 Paulo Alcantara <pc@...guebit.org>,
 Sergey Senozhatsky <senozhatsky@...omium.org>, Tom Talpey <tom@...pey.com>,
 linux-cifs@...r.kernel.org, syzkaller@...glegroups.com,
 linux-kernel@...r.kernel.org, notselwyn@...ing.tech
Subject: Re: [PATCH v3] ksmbd: add kcov remote coverage support via ksmbd_conn

Hi Namjae,

Thank you for the review and the detailed questions.

On 8/19/25 5:00 PM, Namjae Jeon wrote:
> On Wed, Aug 6, 2025 at 11:41 PM Yunseong Kim <ysk@...lloc.com> wrote:
>>
> Hi Yunseong,
>> KSMBD processes SMB requests on per-connection threads and then hands
>> off work items to a kworker pool for actual command processing by
>> handle_ksmbd_work(). Because each connection may enqueue multiple
>> struct ksmbd_work instances, attaching the kcov handle to the work
>> itself is not sufficient: we need a stable, per-connection handle.
>>
>> Introduce a kcov_handle field on struct ksmbd_conn (under CONFIG_KCOV)
>> and initialize it when the connection is set up. In both
>> ksmbd_conn_handler_loop() which only receives a struct ksmbd_conn*
>> and handle_ksmbd_work() which receives a struct ksmbd_work*, start
>> kcov_remote with the per-connection handle before processing and stop
>> it afterward. This ensures coverage collection remains active across
>> the entire asynchronous path of each SMB request.
> I'm a bit unclear on the overall impact. Do you have the goal to measure
> the code coverage of all ksmbd components ?

Yes, exactly. The ultimate goal is to effectively fuzz ksmbd using
syzkaller. To achieve this, it is essential to maximize the code coverage
of the core components involved in handling SMB requests.

The primary motivation for this patch is to address the limitations of KCOV
within ksmbd's asynchronous architecture. Ksmbd receives requests on
connection threads but offloads the actual command processing to a kworker
pool via handle_ksmbd_work(). Previously, the coverage from code executed
in the worker threads was not associated with the initial connection's KCOV
handle, resulting in lost coverage data.

By introducing a stable KCOV handle on struct ksmbd_conn, this patch
ensures that code coverage is accurately tracked across the entire
execution path of an SMB request, from reception to the completion of
asynchronous processing. This is the key impact of this change.

> Is there the next patch set or any plan for next work, or is this patch enough
> to check all functions of ksmbd with syzkaller?

This patch provides the necessary kernel-side infrastructure required to
collect accurate coverage data. It is a crucial prerequisite.

However, this patch alone is not sufficient to check all ksmbd functions.
To actually exercise and test ksmbd's functionalities, corresponding
user-space support in syzkaller (such as syscall descriptions) is required
o leverage this infrastructure. As mentioned in the commit message, that
work is currently in progress here:

Link: https://github.com/google/syzkaller/pull/5524

I am currently investigating cases where Samba, as you previously mentioned,
is mounted on the client and actually writing files exceeds the permissions.
In this process, I discovered a potentially serious vulnerability in netfs.
I have separately reported this issue privately to David Howells.

> Thanks.

In summary, both this kernel patch and the ongoing syzkaller PR need to be
merged to enable effective fuzzing and coverage analysis of ksmbd.

>> The kcov context tied to the connection itself, correctly supporting
>> multiple outstanding work items per connection.
>>
>> In patch v2, I added the missing initialization of kcov_handle. In v3,
>> I fixed an kcov_hanlde argument was previously unused on
>> ksmbd_conn_set_kcov_handle().
>>
>> The related work for syzkaller support is currently being developed
>> in the following GitHub PR:
>> Link: https://github.com/google/syzkaller/pull/5524
>>
>> Based on earlier work by Lau.
>> Link: https://pwning.tech/ksmbd-syzkaller/
>>
>> Cc: linux-cifs@...r.kernel.org
>> Cc: notselwyn@...ing.tech
>> Signed-off-by: Yunseong Kim <ysk@...lloc.com>
>> ---
>>  fs/smb/server/connection.c |  7 ++++++-
>>  fs/smb/server/connection.h | 22 ++++++++++++++++++++++
>>  fs/smb/server/server.c     |  4 ++++
>>  3 files changed, 32 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
>> index 3f04a2977ba8..21352f37384f 100644
>> --- a/fs/smb/server/connection.c
>> +++ b/fs/smb/server/connection.c
>> @@ -93,6 +93,9 @@ struct ksmbd_conn *ksmbd_conn_alloc(void)
>>         down_write(&conn_list_lock);
>>         list_add(&conn->conns_list, &conn_list);
>>         up_write(&conn_list_lock);
>> +
>> +       ksmbd_conn_set_kcov_handle(conn, kcov_common_handle());
>> +
>>         return conn;
>>  }
>>
>> @@ -322,6 +325,8 @@ int ksmbd_conn_handler_loop(void *p)
>>         if (t->ops->prepare && t->ops->prepare(t))
>>                 goto out;
>>
>> +       kcov_remote_start_common(ksmbd_conn_get_kcov_handle(conn));
>> +
>>         max_req = server_conf.max_inflight_req;
>>         conn->last_active = jiffies;
>>         set_freezable();
>> @@ -412,7 +417,7 @@ int ksmbd_conn_handler_loop(void *p)
>>                         break;
>>                 }
>>         }
>> -
>> +       kcov_remote_stop();
>>  out:
>>         ksmbd_conn_set_releasing(conn);
>>         /* Wait till all reference dropped to the Server object*/
>> diff --git a/fs/smb/server/connection.h b/fs/smb/server/connection.h
>> index dd3e0e3f7bf0..a90bd1b3e1df 100644
>> --- a/fs/smb/server/connection.h
>> +++ b/fs/smb/server/connection.h
>> @@ -15,6 +15,7 @@
>>  #include <linux/kthread.h>
>>  #include <linux/nls.h>
>>  #include <linux/unicode.h>
>> +#include <linux/kcov.h>
>>
>>  #include "smb_common.h"
>>  #include "ksmbd_work.h"
>> @@ -109,6 +110,9 @@ struct ksmbd_conn {
>>         bool                            binding;
>>         atomic_t                        refcnt;
>>         bool                            is_aapl;
>> +#ifdef CONFIG_KCOV
>> +       u64                             kcov_handle;
>> +#endif
>>  };
>>
>>  struct ksmbd_conn_ops {
>> @@ -246,4 +250,22 @@ static inline void ksmbd_conn_set_releasing(struct ksmbd_conn *conn)
>>  }
>>
>>  void ksmbd_all_conn_set_status(u64 sess_id, u32 status);
>> +
>> +static inline void ksmbd_conn_set_kcov_handle(struct ksmbd_conn *conn,
>> +                                      const u64 kcov_handle)
>> +{
>> +#ifdef CONFIG_KCOV
>> +       conn->kcov_handle = kcov_handle;
>> +#endif
>> +}
>> +
>> +static inline u64 ksmbd_conn_get_kcov_handle(struct ksmbd_conn *conn)
>> +{
>> +#ifdef CONFIG_KCOV
>> +       return conn->kcov_handle;
>> +#else
>> +       return 0;
>> +#endif
>> +}
>> +
>>  #endif /* __CONNECTION_H__ */
>> diff --git a/fs/smb/server/server.c b/fs/smb/server/server.c
>> index 8c9c49c3a0a4..0757cd6ef4f7 100644
>> --- a/fs/smb/server/server.c
>> +++ b/fs/smb/server/server.c
>> @@ -264,6 +264,8 @@ static void handle_ksmbd_work(struct work_struct *wk)
>>         struct ksmbd_work *work = container_of(wk, struct ksmbd_work, work);
>>         struct ksmbd_conn *conn = work->conn;
>>
>> +       kcov_remote_start_common(ksmbd_conn_get_kcov_handle(conn));
>> +
>>         atomic64_inc(&conn->stats.request_served);
>>
>>         __handle_ksmbd_work(work, conn);
>> @@ -271,6 +273,8 @@ static void handle_ksmbd_work(struct work_struct *wk)
>>         ksmbd_conn_try_dequeue_request(work);
>>         ksmbd_free_work_struct(work);
>>         ksmbd_conn_r_count_dec(conn);
>> +
>> +       kcov_remote_stop();
>>  }
>>
>>  /**
>> --
>> 2.50.0
>>
Please let me know if you have any further questions.

Best regards,
Yunseong Kim.

Powered by blists - more mailing lists