Recently we committed a fix to allow processes to receive notifications for non-zero exits via the process connector module. Commit is a4c9a56e6a2c.
However, for threads, when it does a pthread_exit(&exit_status) call, the kernel is not aware of the exit status with which pthread_exit is called. It is sent by child thread to the parent process, if it is waiting in pthread_join(). Hence, for a thread exiting abnormally, kernel cannot send notifications to any listening processes.
The exception to this is if the thread is sent a signal which it has not handled, and dies along with it's process as a result; for eg. SIGSEGV or SIGKILL. In this case, kernel is aware of the non-zero exit and sends a notification for it.
For our use case, we cannot have parent wait in pthread_join, one of the main reasons for this being that we do not want to track normal pthread_exit(), which could be a very large number. We only want to be notified of any abnormal exits. Hence, threads are created with pthread_attr_t set to PTHREAD_CREATE_DETACHED.
To fix this problem, we add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a thread to send it's exit status to kernel either when it needs to call pthread_exit() with non-zero value to indicate some error or from signal handler before pthread_exit().
We also need to filter packets with non-zero exit notifications futher based on instances, which can be identified by task names. Hence, added a comm field to the packet's struct proc_event, in which task->comm is stored.
v4->v5 changes: - Handled comment by Stanislav Fomichev to fix a print format error. - Made thread.c completely automated by starting proc_filter program from within threads.c. - Changed name CONFIG_CN_HASH_KUNIT_TEST to CN_HASH_KUNIT_TEST in Kconfig.debug and changed display text.
v3->v4 changes: - Reduce size of exit.log by removing unnecessary text.
v2->v3 changes: - Handled comment by Liam Howlett to set hdev to NULL and add comment on it. - Handled comment by Liam Howlett to combine functions for deleting+get and deleting into one in cn_hash.c - Handled comment by Liam Howlett to remove extern in the functions defined in cn_hash_test.h - Some nits by Liam Howlett fixed. - Handled comment by Liam Howlett to make threads test automated. proc_filter.c creates exit.log, which is read by thread.c and checks the values reported. - Added "comm" field to struct proc_event, to copy the task's name to the packet to allow further filtering by packets.
v1->v2 changes: - Handled comment by Peter Zijlstra to remove locking for PF_EXIT_NOTIFY task->flags. - Added error handling in thread.c
v->v1 changes: - Handled comment by Simon Horman to remove unused err in cn_proc.c - Handled comment by Simon Horman to make adata and key_display static in cn_hash_test.c
Anjali Kulkarni (3): connector/cn_proc: Add hash table for threads connector/cn_proc: Kunit tests for threads hash table connector/cn_proc: Selftest for threads
drivers/connector/Makefile | 2 +- drivers/connector/cn_hash.c | 221 +++++++++++++++++ drivers/connector/cn_proc.c | 62 ++++- drivers/connector/connector.c | 75 +++++- include/linux/connector.h | 35 +++ include/linux/sched.h | 2 +- include/uapi/linux/cn_proc.h | 5 +- lib/Kconfig.debug | 17 ++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 +++++++++++++ lib/cn_hash_test.h | 10 + tools/testing/selftests/connector/Makefile | 23 +- .../testing/selftests/connector/proc_filter.c | 34 ++- tools/testing/selftests/connector/thread.c | 232 ++++++++++++++++++ .../selftests/connector/thread_filter.c | 96 ++++++++ 15 files changed, 967 insertions(+), 15 deletions(-) create mode 100644 drivers/connector/cn_hash.c create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h create mode 100644 tools/testing/selftests/connector/thread.c create mode 100644 tools/testing/selftests/connector/thread_filter.c
Add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a thread to notify the kernel that is going to exit with a non-zero exit code and specify the exit code in it. When thread exits in the kernel, it will send this exit code as a proc filter notification to any listening process. Exiting thread can call this either when it wants to call pthread_exit() with non-zero value or from signal handler.
Add a new file cn_hash.c which implements a hash table storing the exit codes of abnormally exiting threads, received by the system call above. The key used for the hash table is the pid of the thread, so when the thread actually exits, we lookup it's pid in the hash table and retrieve the exit code sent by user. If the exit code in struct task is 0, we then replace it with the user supplied non-zero exit code.
cn_hash.c implements the hash table add, delete, lookup operations. mutex_lock() and mutex_unlock() operations are used to safeguard the integrity of the hash table while adding or deleting elements. connector.c has the API calls, called from cn_proc.c, as well as calls to allocate, initialize and free the hash table.
Add a new flag in PF_* flags of task_struct - EXIT_NOTIFY. This flag is set when user sends the exit code via PROC_CN_MCAST_NOTIFY. While exiting, this flag is checked and the hash table add or delete calls are only made if this flag is set.
A refcount field hrefcnt is added in struct cn_hash_dev, to keep track of number of threads which have added an entry in hash table. Before freeing the struct cn_hash_dev, this value must be 0. This refcnt check is added in case CONFIG_CONNECTOR is compiled as a module. In that case, when unloading the module, we need to make sure no hash entries are still present in the hdev table.
Copy the task's name (task->comm) into the exit event notification. This will allow applications to filter on the name further using userspace filtering like ebpf.
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com --- drivers/connector/Makefile | 2 +- drivers/connector/cn_hash.c | 181 ++++++++++++++++++++++++++++++++++ drivers/connector/cn_proc.c | 62 +++++++++++- drivers/connector/connector.c | 63 +++++++++++- include/linux/connector.h | 31 ++++++ include/linux/sched.h | 2 +- include/uapi/linux/cn_proc.h | 5 +- 7 files changed, 338 insertions(+), 8 deletions(-) create mode 100644 drivers/connector/cn_hash.c
diff --git a/drivers/connector/Makefile b/drivers/connector/Makefile index 1bf67d3df97d..cb1dcdf067ad 100644 --- a/drivers/connector/Makefile +++ b/drivers/connector/Makefile @@ -2,4 +2,4 @@ obj-$(CONFIG_CONNECTOR) += cn.o obj-$(CONFIG_PROC_EVENTS) += cn_proc.o
-cn-y += cn_queue.o connector.o +cn-y += cn_hash.o cn_queue.o connector.o diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c new file mode 100644 index 000000000000..a079e9bcea6d --- /dev/null +++ b/drivers/connector/cn_hash.c @@ -0,0 +1,181 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + * + * Copyright (c) 2024 Oracle and/or its affiliates. + */ + +#include <linux/kernel.h> +#include <linux/init.h> +#include <linux/connector.h> +#include <linux/mutex.h> +#include <linux/pid_namespace.h> + +#include <linux/cn_proc.h> + +struct cn_hash_dev *cn_hash_alloc_dev(const char *name) +{ + struct cn_hash_dev *hdev; + + hdev = kzalloc(sizeof(*hdev), GFP_KERNEL); + if (!hdev) + return NULL; + + snprintf(hdev->name, sizeof(hdev->name), "%s", name); + atomic_set(&hdev->hrefcnt, 0); + mutex_init(&hdev->uexit_hash_lock); + hash_init(hdev->uexit_pid_htable); + return hdev; +} + +void cn_hash_free_dev(struct cn_hash_dev *hdev) +{ + struct uexit_pid_hnode *hnode; + struct hlist_node *tmp; + int bucket; + + pr_debug("%s: Freeing entire hdev %p\n", __func__, hdev); + + mutex_lock(&hdev->uexit_hash_lock); + hash_for_each_safe(hdev->uexit_pid_htable, bucket, tmp, + hnode, uexit_pid_hlist) { + hash_del(&hnode->uexit_pid_hlist); + pr_debug("%s: Freeing node for pid %d\n", + __func__, hnode->pid); + kfree(hnode); + } + + mutex_unlock(&hdev->uexit_hash_lock); + mutex_destroy(&hdev->uexit_hash_lock); + + /* + * This refcnt check is added in case CONFIG_CONNECTOR is + * compiled with =m as a module. In that case, when unloading + * the module, we need to make sure no hash entries are still + * present in the hdev table. + */ + while (atomic_read(&hdev->hrefcnt)) { + pr_info("Waiting for %s to become free: refcnt=%d\n", + hdev->name, atomic_read(&hdev->hrefcnt)); + msleep(1000); + } + + kfree(hdev); + hdev = NULL; +} + +static struct uexit_pid_hnode *cn_hash_alloc_elem(__u32 uexit_code, pid_t pid) +{ + struct uexit_pid_hnode *elem; + + elem = kzalloc(sizeof(*elem), GFP_KERNEL); + if (!elem) + return NULL; + + INIT_HLIST_NODE(&elem->uexit_pid_hlist); + elem->uexit_code = uexit_code; + elem->pid = pid; + return elem; +} + +static inline void cn_hash_free_elem(struct uexit_pid_hnode *elem) +{ + kfree(elem); +} + +int cn_hash_add_elem(struct cn_hash_dev *hdev, __u32 uexit_code, pid_t pid) +{ + struct uexit_pid_hnode *elem, *hnode; + + elem = cn_hash_alloc_elem(uexit_code, pid); + if (!elem) { + pr_err("%s: cn_hash_alloc_elem() returned NULL pid %d\n", + __func__, pid); + return -ENOMEM; + } + + mutex_lock(&hdev->uexit_hash_lock); + /* + * Check if an entry for the same pid already exists + */ + hash_for_each_possible(hdev->uexit_pid_htable, + hnode, uexit_pid_hlist, pid) { + if (hnode->pid == pid) { + mutex_unlock(&hdev->uexit_hash_lock); + cn_hash_free_elem(elem); + pr_debug("%s: pid %d already exists in hash table\n", + __func__, pid); + return -EEXIST; + } + } + + hash_add(hdev->uexit_pid_htable, &elem->uexit_pid_hlist, pid); + mutex_unlock(&hdev->uexit_hash_lock); + + atomic_inc(&hdev->hrefcnt); + + pr_debug("%s: After hash_add of pid %d elem %p hrefcnt %d\n", + __func__, pid, elem, atomic_read(&hdev->hrefcnt)); + return 0; +} + +int cn_hash_del_get_exval(struct cn_hash_dev *hdev, pid_t pid) +{ + struct uexit_pid_hnode *hnode; + struct hlist_node *tmp; + int excde; + + mutex_lock(&hdev->uexit_hash_lock); + hash_for_each_possible_safe(hdev->uexit_pid_htable, + hnode, tmp, uexit_pid_hlist, pid) { + if (hnode->pid == pid) { + excde = hnode->uexit_code; + hash_del(&hnode->uexit_pid_hlist); + mutex_unlock(&hdev->uexit_hash_lock); + kfree(hnode); + atomic_dec(&hdev->hrefcnt); + pr_debug("%s: After hash_del of pid %d, found exit code %u hrefcnt %d\n", + __func__, pid, excde, + atomic_read(&hdev->hrefcnt)); + return excde; + } + } + + mutex_unlock(&hdev->uexit_hash_lock); + pr_err("%s: pid %d not found in hash table\n", + __func__, pid); + return -EINVAL; +} + +int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) +{ + struct uexit_pid_hnode *hnode; + __u32 excde; + + mutex_lock(&hdev->uexit_hash_lock); + hash_for_each_possible(hdev->uexit_pid_htable, + hnode, uexit_pid_hlist, pid) { + if (hnode->pid == pid) { + excde = hnode->uexit_code; + mutex_unlock(&hdev->uexit_hash_lock); + pr_debug("%s: Found exit code %u for pid %d\n", + __func__, excde, pid); + return excde; + } + } + + mutex_unlock(&hdev->uexit_hash_lock); + pr_debug("%s: pid %d not found in hash table\n", + __func__, pid); + return -EINVAL; +} + +bool cn_hash_table_empty(struct cn_hash_dev *hdev) +{ + bool is_empty; + + is_empty = hash_empty(hdev->uexit_pid_htable); + pr_debug("Hash table is %s\n", (is_empty ? "empty" : "not empty")); + + return is_empty; +} diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c index 44b19e696176..101f1fba5ad9 100644 --- a/drivers/connector/cn_proc.c +++ b/drivers/connector/cn_proc.c @@ -69,6 +69,8 @@ static int cn_filter(struct sock *dsk, struct sk_buff *skb, void *data) if ((__u32)val == PROC_EVENT_ALL) return 0;
+ pr_debug("%s: val %lx, what %x\n", __func__, val, what); + /* * Drop packet if we have to report only non-zero exit status * (PROC_EVENT_NONZERO_EXIT) and exit status is 0 @@ -326,9 +328,15 @@ void proc_exit_connector(struct task_struct *task) struct proc_event *ev; struct task_struct *parent; __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8); + int uexit_code;
- if (atomic_read(&proc_event_num_listeners) < 1) + if (atomic_read(&proc_event_num_listeners) < 1) { + if (likely(!(task->flags & PF_EXIT_NOTIFY))) + return; + + cn_del_get_exval(task->pid); return; + }
msg = buffer_to_cn_msg(buffer); ev = (struct proc_event *)msg->data; @@ -337,7 +345,26 @@ void proc_exit_connector(struct task_struct *task) ev->what = PROC_EVENT_EXIT; ev->event_data.exit.process_pid = task->pid; ev->event_data.exit.process_tgid = task->tgid; - ev->event_data.exit.exit_code = task->exit_code; + if (unlikely(task->flags & PF_EXIT_NOTIFY)) { + task->flags &= ~PF_EXIT_NOTIFY; + + uexit_code = cn_del_get_exval(task->pid); + if (uexit_code <= 0) { + pr_debug("%s: err %d returning task's exit code %u\n", + __func__, uexit_code, + task->exit_code); + ev->event_data.exit.exit_code = task->exit_code; + } else { + ev->event_data.exit.exit_code = uexit_code; + pr_debug("%s: Reset PF_EXIT_NOTIFY & retrieved exit code %u from hash table, pid %d\n", + __func__, + ev->event_data.exit.exit_code, + task->pid); + } + } else { + ev->event_data.exit.exit_code = task->exit_code; + } + ev->event_data.exit.exit_signal = task->exit_signal;
rcu_read_lock(); @@ -348,6 +375,13 @@ void proc_exit_connector(struct task_struct *task) } rcu_read_unlock();
+ /* + * Copy task name in the packet. This will allow applications + * to filter on the name further using userspace filtering like + * ebpf + */ + get_task_comm(ev->event_data.exit.comm, task); + memcpy(&msg->id, &cn_proc_event_id, sizeof(msg->id)); msg->ack = 0; /* not used */ msg->len = sizeof(*ev); @@ -413,6 +447,13 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, if (msg->len == sizeof(*pinput)) { pinput = (struct proc_input *)msg->data; mc_op = pinput->mcast_op; + if (mc_op == PROC_CN_MCAST_NOTIFY) { + pr_debug("%s: Received PROC_CN_MCAST_NOTIFY, pid %d\n", + __func__, current->pid); + current->flags |= PF_EXIT_NOTIFY; + err = cn_add_elem(pinput->uexit_code, current->pid); + return; + } ev_type = pinput->event_type; } else if (msg->len == sizeof(mc_op)) { mc_op = *((enum proc_cn_mcast_op *)msg->data); @@ -432,6 +473,8 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, sk->sk_user_data = kzalloc(sizeof(struct proc_input), GFP_KERNEL); if (sk->sk_user_data == NULL) { + pr_err("%s: ENOMEM for sk_user_data, pid %d\n", + __func__, current->pid); err = ENOMEM; goto out; } @@ -442,21 +485,32 @@ static void cn_proc_mcast_ctl(struct cn_msg *msg, } ((struct proc_input *)(sk->sk_user_data))->event_type = ev_type; + pr_debug("%s: sk: %p pid: %d event_type: %x\n", + __func__, sk, current->pid, ev_type); ((struct proc_input *)(sk->sk_user_data))->mcast_op = mc_op; }
switch (mc_op) { case PROC_CN_MCAST_LISTEN: - if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN)) + if (initial || (prev_mc_op != PROC_CN_MCAST_LISTEN)) { atomic_inc(&proc_event_num_listeners); + pr_debug("%s: PROC_CN_MCAST_LISTEN pid %d: Incremented listeners to %d\n", + __func__, current->pid, + atomic_read(&proc_event_num_listeners)); + } break; case PROC_CN_MCAST_IGNORE: - if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE)) + if (!initial && (prev_mc_op != PROC_CN_MCAST_IGNORE)) { atomic_dec(&proc_event_num_listeners); + pr_debug("%s: PROC_CN_MCAST_IGNORE pid %d: Decremented listeners to %d\n", + __func__, current->pid, + atomic_read(&proc_event_num_listeners)); + } ((struct proc_input *)(sk->sk_user_data))->event_type = PROC_EVENT_NONE; break; default: + pr_warn("%s: Invalid value for mc_op %d\n", __func__, mc_op); err = EINVAL; break; } diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index 4028e8eeba82..c1c0dcec53c0 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -271,6 +271,50 @@ static int __maybe_unused cn_proc_show(struct seq_file *m, void *v) return 0; }
+int cn_del_get_exval(pid_t pid) +{ + struct cn_dev *dev = &cdev; + + if (!cn_already_initialized) + return 0; + + return cn_hash_del_get_exval(dev->hdev, pid); +} +EXPORT_SYMBOL_GPL(cn_del_get_exval); + +int cn_add_elem(__u32 uexit_code, pid_t pid) +{ + struct cn_dev *dev = &cdev; + + if (!cn_already_initialized) + return 0; + + return cn_hash_add_elem(dev->hdev, uexit_code, pid); +} +EXPORT_SYMBOL_GPL(cn_add_elem); + +int cn_get_exval(pid_t pid) +{ + struct cn_dev *dev = &cdev; + + if (!cn_already_initialized) + return 0; + + return cn_hash_get_exval(dev->hdev, pid); +} +EXPORT_SYMBOL_GPL(cn_get_exval); + +bool cn_table_empty(void) +{ + struct cn_dev *dev = &cdev; + + if (!cn_already_initialized) + return 0; + + return cn_hash_table_empty(dev->hdev); +} +EXPORT_SYMBOL_GPL(cn_table_empty); + static int cn_init(void) { struct cn_dev *dev = &cdev; @@ -283,15 +327,31 @@ static int cn_init(void) };
dev->nls = netlink_kernel_create(&init_net, NETLINK_CONNECTOR, &cfg); - if (!dev->nls) + if (!dev->nls) { + pr_err("%s: netlink_kernel_create failed, connector not initialized\n", + __func__); return -EIO; + }
dev->cbdev = cn_queue_alloc_dev("cqueue", dev->nls); if (!dev->cbdev) { + pr_err("%s: Allocation of dev->cbdev failed, connector not initialized\n", + __func__); netlink_kernel_release(dev->nls); return -EINVAL; }
+ dev->hdev = cn_hash_alloc_dev("pid hash table"); + if (!dev->hdev) { + pr_err("%s: Allocation of dev->hdev failed, connector not initialized\n", + __func__); + netlink_kernel_release(dev->nls); + cn_queue_free_dev(dev->cbdev); + return -ENOMEM; + } + + pr_debug("Connector initialized, allocated hdev %p\n", dev->hdev); + cn_already_initialized = 1;
proc_create_single("connector", S_IRUGO, init_net.proc_net, cn_proc_show); @@ -308,6 +368,7 @@ static void cn_fini(void) remove_proc_entry("connector", init_net.proc_net);
cn_queue_free_dev(dev->cbdev); + cn_hash_free_dev(dev->hdev); netlink_kernel_release(dev->nls); }
diff --git a/include/linux/connector.h b/include/linux/connector.h index 70bc1160f3d8..5384e4bb98e8 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -18,6 +18,8 @@ #include <uapi/linux/connector.h>
#define CN_CBQ_NAMELEN 32 +#define HASHT_NAMELEN 32 +#define PID_HASH_TABLE_BITS 10
struct cn_queue_dev { atomic_t refcnt; @@ -45,6 +47,19 @@ struct cn_callback_entry { u32 seq, group; };
+struct uexit_pid_hnode { + __u32 uexit_code; + pid_t pid; + struct hlist_node uexit_pid_hlist; +}; + +struct cn_hash_dev { + atomic_t hrefcnt; + unsigned char name[HASHT_NAMELEN]; + struct mutex uexit_hash_lock; + DECLARE_HASHTABLE(uexit_pid_htable, PID_HASH_TABLE_BITS); +}; + struct cn_dev { struct cb_id id;
@@ -52,6 +67,7 @@ struct cn_dev { struct sock *nls;
struct cn_queue_dev *cbdev; + struct cn_hash_dev *hdev; };
/** @@ -137,4 +153,19 @@ void cn_queue_free_dev(struct cn_queue_dev *dev);
int cn_cb_equal(const struct cb_id *, const struct cb_id *);
+struct cn_hash_dev *cn_hash_alloc_dev(const char *name); +void cn_hash_free_dev(struct cn_hash_dev *hdev); +struct uexit_pid_hnode *cn_hash_find_pid_node(struct cn_hash_dev *hdev, + pid_t pid); +int cn_hash_add_elem(struct cn_hash_dev *hdev, __u32 uexit_code, pid_t pid); +int cn_hash_del_get_exval(struct cn_hash_dev *hdev, pid_t pid); +int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid); + +int cn_add_elem(__u32 uexit_code, pid_t pid); +int cn_del_get_exval(pid_t pid); +int cn_get_exval(pid_t pid); + +bool cn_table_empty(void); +bool cn_hash_table_empty(struct cn_hash_dev *hdev); + #endif /* __CONNECTOR_H */ diff --git a/include/linux/sched.h b/include/linux/sched.h index e6ee4258169a..a2339ae6208b 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1673,7 +1673,7 @@ extern struct pid *cad_pid; #define PF_USED_MATH 0x00002000 /* If unset the fpu must be initialized before use */ #define PF_USER_WORKER 0x00004000 /* Kernel thread cloned from userspace thread */ #define PF_NOFREEZE 0x00008000 /* This thread should not be frozen */ -#define PF__HOLE__00010000 0x00010000 +#define PF_EXIT_NOTIFY 0x00010000 /* This thread has sent an exit value to be sent as a notification to listening processes */ #define PF_KSWAPD 0x00020000 /* I am kswapd */ #define PF_MEMALLOC_NOFS 0x00040000 /* All allocations inherit GFP_NOFS. See memalloc_nfs_save() */ #define PF_MEMALLOC_NOIO 0x00080000 /* All allocations inherit GFP_NOIO. See memalloc_noio_save() */ diff --git a/include/uapi/linux/cn_proc.h b/include/uapi/linux/cn_proc.h index 18e3745b86cd..e0ddb2e2c069 100644 --- a/include/uapi/linux/cn_proc.h +++ b/include/uapi/linux/cn_proc.h @@ -27,7 +27,8 @@ */ enum proc_cn_mcast_op { PROC_CN_MCAST_LISTEN = 1, - PROC_CN_MCAST_IGNORE = 2 + PROC_CN_MCAST_IGNORE = 2, + PROC_CN_MCAST_NOTIFY = 3 };
#define PROC_EVENT_ALL (PROC_EVENT_FORK | PROC_EVENT_EXEC | PROC_EVENT_UID | \ @@ -65,6 +66,7 @@ enum proc_cn_event { struct proc_input { enum proc_cn_mcast_op mcast_op; enum proc_cn_event event_type; + __u32 uexit_code; };
static inline enum proc_cn_event valid_event(enum proc_cn_event ev_type) @@ -151,6 +153,7 @@ struct proc_event { __u32 exit_code, exit_signal; __kernel_pid_t parent_pid; __kernel_pid_t parent_tgid; + char comm[16]; } exit;
} event_data;
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com --- drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; }
+int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len, + int *hkey, int *key_display) +{ + struct uexit_pid_hnode *hnode; + int key, count = 0; + + mutex_lock(&hdev->uexit_hash_lock); + key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable)); + pr_debug("Bucket: %d\n", key); + + hlist_for_each_entry(hnode, + &hdev->uexit_pid_htable[key], + uexit_pid_hlist) { + if (key_display[key] != 1) { + if (hnode->uexit_pid_hlist.next == NULL) + pr_debug("pid %d ", hnode->pid); + else + pr_debug("pid %d --> ", hnode->pid); + } + count++; + } + + mutex_unlock(&hdev->uexit_hash_lock); + + if ((key_display[key] != 1) && !count) + pr_debug("(empty)\n"); + + pr_debug("\n"); + + *hkey = key; + + if (count > max_len) { + pr_err("%d entries in hlist for key %d, expected %d\n", + count, key, max_len); + return -EINVAL; + } + + return 0; +} + bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{ + struct cn_dev *dev = &cdev; + + if (!cn_already_initialized) + return 0; + + return cn_hash_display_hlist(dev->hdev, pid, max_len, + hkey, key_display); +} +EXPORT_SYMBOL_GPL(cn_display_hlist); + bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len, + int *hkey, int *key_display); + #endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
If unsure, say N.
+config CN_HASH_KUNIT_TEST + tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS + depends on KUNIT + default KUNIT_ALL_TESTS + help + This builds the hashtable KUnit test suite. + It tests the basic functionality of the API defined in + drivers/connector/cn_hash.c. + CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needs + to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and + CONFIG_KUNIT=m in .config file to compile and then test as a kernel + module with "modprobe cn_hash_test". + For more information on KUnit and unit tests in general please + refer to the KUnit documentation in Documentation/dev-tools/kunit/. + + If unsure, say N. + config LINEAR_RANGES_TEST tristate "KUnit test for linear_ranges" depends on KUNIT diff --git a/lib/Makefile b/lib/Makefile index 811ba12c8cd0..2c59c82b0b18 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -379,6 +379,7 @@ obj-$(CONFIG_CMDLINE_KUNIT_TEST) += cmdline_kunit.o obj-$(CONFIG_SLUB_KUNIT_TEST) += slub_kunit.o obj-$(CONFIG_MEMCPY_KUNIT_TEST) += memcpy_kunit.o obj-$(CONFIG_IS_SIGNED_TYPE_KUNIT_TEST) += is_signed_type_kunit.o +obj-$(CONFIG_CN_HASH_KUNIT_TEST) += cn_hash_test.o CFLAGS_overflow_kunit.o = $(call cc-disable-warning, tautological-constant-out-of-range-compare) obj-$(CONFIG_OVERFLOW_KUNIT_TEST) += overflow_kunit.o CFLAGS_stackinit_kunit.o += $(call cc-disable-warning, switch-unreachable) diff --git a/lib/cn_hash_test.c b/lib/cn_hash_test.c new file mode 100644 index 000000000000..f90989343468 --- /dev/null +++ b/lib/cn_hash_test.c @@ -0,0 +1,167 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * KUnit test for the connector threads hashtable code. + * + * Copyright (c) 2024 Oracle and/or its affiliates. + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + */ +#include <kunit/test.h> + +#include "cn_hash_test.h" + +#define ARR_SIZE 4 +#define HASH_TABLE_LEN 1024 + +struct add_data { + pid_t pid; + int exit_val; + int key; +}; + +static struct add_data adata[ARR_SIZE]; +static int key_display[HASH_TABLE_LEN]; + +static int cn_hash_init(struct kunit *test) +{ + for (int i = 0; i < HASH_TABLE_LEN; i++) + key_display[i] = 0; + + return 0; +} + +static void cn_display_htable(struct kunit *test, int len) +{ + int i, err; + + cn_hash_init(test); + + pr_debug("\n"); + pr_debug("Displaying hash table:\n"); + + for (i = 0; i < len; i++) { + err = cn_display_hlist(adata[i].pid, len, &adata[i].key, + key_display); + key_display[adata[i].key] = 1; + KUNIT_EXPECT_EQ(test, err, 0); + } +} + +static void cn_hash_test_add(struct kunit *test) +{ + int err, i; + int exit_val; + + adata[0].pid = 1; + adata[0].exit_val = 45; + + adata[1].pid = 2; + adata[1].exit_val = 13; + + adata[2].pid = 1024; + adata[2].exit_val = 16; + + adata[3].pid = 1023; + adata[3].exit_val = 71; + + for (i = 0; i < ARRAY_SIZE(adata); i++) { + err = cn_add_elem(adata[i].exit_val, adata[i].pid); + KUNIT_EXPECT_EQ_MSG(test, 0, err, + "Adding pid %d returned err %d", + adata[i].pid, err); + + exit_val = cn_get_exval(adata[i].pid); + KUNIT_EXPECT_EQ(test, adata[i].exit_val, exit_val); + } + + cn_display_htable(test, ARRAY_SIZE(adata)); +} + +static void cn_hash_test_del(struct kunit *test) +{ + int i, err; + int exit_val; + + for (i = 0; i < ARRAY_SIZE(adata); i++) { + err = cn_del_get_exval(adata[i].pid); + KUNIT_EXPECT_GT_MSG(test, err, 0, + "Deleting pid %d returned err %d", + adata[i].pid, err); + + exit_val = cn_get_exval(adata[i].pid); + KUNIT_EXPECT_EQ(test, -EINVAL, exit_val); + } + + cn_display_htable(test, ARRAY_SIZE(adata)); + KUNIT_EXPECT_TRUE(test, cn_table_empty()); +} + +static void cn_hash_test_del_get_exval(struct kunit *test) +{ + int i, exval; + + for (i = 0; i < ARRAY_SIZE(adata); i++) { + exval = cn_del_get_exval(adata[i].pid); + KUNIT_EXPECT_EQ(test, adata[i].exit_val, exval); + + cn_display_htable(test, ARRAY_SIZE(adata)); + } + + KUNIT_EXPECT_TRUE(test, cn_table_empty()); +} +static void cn_hash_test_dup_add(struct kunit *test) +{ + int err, exit_val; + + adata[0].pid = 10; + adata[0].exit_val = 21; + + err = cn_add_elem(adata[0].exit_val, adata[0].pid); + KUNIT_EXPECT_EQ(test, 0, err); + + exit_val = cn_get_exval(adata[0].pid); + KUNIT_EXPECT_EQ(test, 21, exit_val); + + adata[1].pid = 10; + adata[1].exit_val = 12; + + err = cn_add_elem(adata[1].exit_val, adata[1].pid); + KUNIT_EXPECT_EQ(test, -EEXIST, err); + + exit_val = cn_get_exval(adata[1].pid); + KUNIT_EXPECT_EQ(test, 21, exit_val); + + cn_display_htable(test, 1); +} + +static void cn_hash_test_dup_del(struct kunit *test) +{ + int err; + + err = cn_del_get_exval(adata[0].pid); + KUNIT_EXPECT_EQ(test, adata[0].exit_val, err); + + err = cn_del_get_exval(adata[0].pid); + KUNIT_EXPECT_EQ(test, -EINVAL, err); + + KUNIT_EXPECT_TRUE(test, cn_table_empty()); +} + +static struct kunit_case cn_hashtable_test_cases[] = { + KUNIT_CASE(cn_hash_test_add), + KUNIT_CASE(cn_hash_test_del), + KUNIT_CASE(cn_hash_test_dup_add), + KUNIT_CASE(cn_hash_test_dup_del), + KUNIT_CASE(cn_hash_test_add), + KUNIT_CASE(cn_hash_test_del_get_exval), + {}, +}; + +static struct kunit_suite cn_hashtable_test_module = { + .name = "cn_hashtable", + .init = cn_hash_init, + .test_cases = cn_hashtable_test_cases, +}; +kunit_test_suite(cn_hashtable_test_module); + +MODULE_DESCRIPTION("KUnit test for the connector threads hashtable code"); +MODULE_LICENSE("GPL"); diff --git a/lib/cn_hash_test.h b/lib/cn_hash_test.h new file mode 100644 index 000000000000..b25033feab09 --- /dev/null +++ b/lib/cn_hash_test.h @@ -0,0 +1,10 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2024 Oracle and/or its affiliates. + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + */ +int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_add_elem(__u32 uexit_code, pid_t pid); +int cn_del_get_exval(pid_t pid); +int cn_get_exval(pid_t pid); +bool cn_table_empty(void);
On 10/17, Anjali Kulkarni wrote:
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; } +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
int *hkey, int *key_display)+{
- struct uexit_pid_hnode *hnode;
- int key, count = 0;
- mutex_lock(&hdev->uexit_hash_lock);
- key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
- pr_debug("Bucket: %d\n", key);
- hlist_for_each_entry(hnode,
&hdev->uexit_pid_htable[key],uexit_pid_hlist) {if (key_display[key] != 1) {if (hnode->uexit_pid_hlist.next == NULL)pr_debug("pid %d ", hnode->pid);elsepr_debug("pid %d --> ", hnode->pid);}count++;- }
- mutex_unlock(&hdev->uexit_hash_lock);
- if ((key_display[key] != 1) && !count)
pr_debug("(empty)\n");- pr_debug("\n");
- *hkey = key;
- if (count > max_len) {
pr_err("%d entries in hlist for key %d, expected %d\n",count, key, max_len);return -EINVAL;- }
- return 0;
+}
bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval); +int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{
- struct cn_dev *dev = &cdev;
- if (!cn_already_initialized)
return 0;- return cn_hash_display_hlist(dev->hdev, pid, max_len,
hkey, key_display);+} +EXPORT_SYMBOL_GPL(cn_display_hlist);
bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev); +int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
int *hkey, int *key_display);#endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST If unsure, say N. +config CN_HASH_KUNIT_TEST
- tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
This builds the hashtable KUnit test suite.It tests the basic functionality of the API defined indrivers/connector/cn_hash.c.CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needsto be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m andCONFIG_KUNIT=m in .config file to compile and then test as a kernelmodule with "modprobe cn_hash_test".For more information on KUnit and unit tests in general pleaserefer to the KUnit documentation in Documentation/dev-tools/kunit/.If unsure, say N.
Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the existing kunit tester complains about the missing symbols (see below). Please also hold off reposting for a couple of days to give people some time to review.
ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del': cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_display_htable': cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist' ld: vmlinux.o: in function `cn_hash_test_del_get_exval': cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_dup_add': cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval' ld: vmlinux.o: in function `cn_hash_test_del': cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_add': cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval' make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1 make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2 make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2 make: *** [Makefile:224: __sub-make] Error 2
--- pw-bot: cr
On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/17, Anjali Kulkarni wrote:
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; }
+int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display)
+{
- struct uexit_pid_hnode *hnode;
- int key, count = 0;
- mutex_lock(&hdev->uexit_hash_lock);
- key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
- pr_debug("Bucket: %d\n", key);
- hlist_for_each_entry(hnode,
- &hdev->uexit_pid_htable[key],
- uexit_pid_hlist) {
- if (key_display[key] != 1) {
- if (hnode->uexit_pid_hlist.next == NULL)
- pr_debug("pid %d ", hnode->pid);
- else
- pr_debug("pid %d --> ", hnode->pid);
- }
- count++;
- }
- mutex_unlock(&hdev->uexit_hash_lock);
- if ((key_display[key] != 1) && !count)
- pr_debug("(empty)\n");
- pr_debug("\n");
- *hkey = key;
- if (count > max_len) {
- pr_err("%d entries in hlist for key %d, expected %d\n",
- count, key, max_len);
- return -EINVAL;
- }
- return 0;
+}
bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{
- struct cn_dev *dev = &cdev;
- if (!cn_already_initialized)
- return 0;
- return cn_hash_display_hlist(dev->hdev, pid, max_len,
- hkey, key_display);
+} +EXPORT_SYMBOL_GPL(cn_display_hlist);
bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display);
#endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
If unsure, say N.
+config CN_HASH_KUNIT_TEST
- tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
- This builds the hashtable KUnit test suite.
- It tests the basic functionality of the API defined in
- drivers/connector/cn_hash.c.
- CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needs
- to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and
- CONFIG_KUNIT=m in .config file to compile and then test as a kernel
- module with "modprobe cn_hash_test".
- For more information on KUnit and unit tests in general please
- refer to the KUnit documentation in Documentation/dev-tools/kunit/.
- If unsure, say N.
Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the existing kunit tester complains about the missing symbols (see below). Please also hold off reposting for a couple of days to give people some time to review.
ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del': cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_display_htable': cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist' ld: vmlinux.o: in function `cn_hash_test_del_get_exval': cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_dup_add': cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval' ld: vmlinux.o: in function `cn_hash_test_del': cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_add': cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval' make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1 make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2 make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2 make: *** [Makefile:224: __sub-make] Error 2
Yes, I have added in the comments for CN_HASH_KUNIT_TEST, it depends on: CONFIG_CONNECTOR, CONFIG_PROC_EVENTS, CONFIG_NET. I didn’t realize I could add these to the “depends” field. So something like this: (let me know if you see any issues)
tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS depends on KUNIT + depends on CONNECTOR && PROC_EVENTS + depends on NET default KUNIT_ALL_TESTS
These are the configs I add to my .config file and compile it as a module and then do modprobe to test. Are you running the kunit tester with kunit.py?
Thanks Anjali
pw-bot: cr
On 10/18, Anjali Kulkarni wrote:
On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/17, Anjali Kulkarni wrote:
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; }
+int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display)
+{
- struct uexit_pid_hnode *hnode;
- int key, count = 0;
- mutex_lock(&hdev->uexit_hash_lock);
- key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
- pr_debug("Bucket: %d\n", key);
- hlist_for_each_entry(hnode,
- &hdev->uexit_pid_htable[key],
- uexit_pid_hlist) {
- if (key_display[key] != 1) {
- if (hnode->uexit_pid_hlist.next == NULL)
- pr_debug("pid %d ", hnode->pid);
- else
- pr_debug("pid %d --> ", hnode->pid);
- }
- count++;
- }
- mutex_unlock(&hdev->uexit_hash_lock);
- if ((key_display[key] != 1) && !count)
- pr_debug("(empty)\n");
- pr_debug("\n");
- *hkey = key;
- if (count > max_len) {
- pr_err("%d entries in hlist for key %d, expected %d\n",
- count, key, max_len);
- return -EINVAL;
- }
- return 0;
+}
bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{
- struct cn_dev *dev = &cdev;
- if (!cn_already_initialized)
- return 0;
- return cn_hash_display_hlist(dev->hdev, pid, max_len,
- hkey, key_display);
+} +EXPORT_SYMBOL_GPL(cn_display_hlist);
bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display);
#endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
If unsure, say N.
+config CN_HASH_KUNIT_TEST
- tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
- This builds the hashtable KUnit test suite.
- It tests the basic functionality of the API defined in
- drivers/connector/cn_hash.c.
- CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needs
- to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and
- CONFIG_KUNIT=m in .config file to compile and then test as a kernel
- module with "modprobe cn_hash_test".
- For more information on KUnit and unit tests in general please
- refer to the KUnit documentation in Documentation/dev-tools/kunit/.
- If unsure, say N.
Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the existing kunit tester complains about the missing symbols (see below). Please also hold off reposting for a couple of days to give people some time to review.
ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del': cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_display_htable': cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist' ld: vmlinux.o: in function `cn_hash_test_del_get_exval': cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_dup_add': cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval' ld: vmlinux.o: in function `cn_hash_test_del': cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_add': cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval' make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1 make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2 make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2 make: *** [Makefile:224: __sub-make] Error 2
Yes, I have added in the comments for CN_HASH_KUNIT_TEST, it depends on: CONFIG_CONNECTOR, CONFIG_PROC_EVENTS, CONFIG_NET. I didn’t realize I could add these to the “depends” field. So something like this: (let me know if you see any issues)
tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS depends on KUNIT
depends on CONNECTOR && PROC_EVENTSdepends on NET default KUNIT_ALL_TESTSThese are the configs I add to my .config file and compile it as a module and then do modprobe to test.
[..]
Are you running the kunit tester with kunit.py?
Yes, make sure all required options are picked up by "./tools/testing/kunit/kunit.py run" instead of manually adding options and doing modprobe.
On Oct 17, 2024, at 5:55 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/18, Anjali Kulkarni wrote:
On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/17, Anjali Kulkarni wrote:
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; }
+int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display)
+{
- struct uexit_pid_hnode *hnode;
- int key, count = 0;
- mutex_lock(&hdev->uexit_hash_lock);
- key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
- pr_debug("Bucket: %d\n", key);
- hlist_for_each_entry(hnode,
- &hdev->uexit_pid_htable[key],
- uexit_pid_hlist) {
- if (key_display[key] != 1) {
- if (hnode->uexit_pid_hlist.next == NULL)
- pr_debug("pid %d ", hnode->pid);
- else
- pr_debug("pid %d --> ", hnode->pid);
- }
- count++;
- }
- mutex_unlock(&hdev->uexit_hash_lock);
- if ((key_display[key] != 1) && !count)
- pr_debug("(empty)\n");
- pr_debug("\n");
- *hkey = key;
- if (count > max_len) {
- pr_err("%d entries in hlist for key %d, expected %d\n",
- count, key, max_len);
- return -EINVAL;
- }
- return 0;
+}
bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{
- struct cn_dev *dev = &cdev;
- if (!cn_already_initialized)
- return 0;
- return cn_hash_display_hlist(dev->hdev, pid, max_len,
- hkey, key_display);
+} +EXPORT_SYMBOL_GPL(cn_display_hlist);
bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display);
#endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
If unsure, say N.
+config CN_HASH_KUNIT_TEST
- tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
- This builds the hashtable KUnit test suite.
- It tests the basic functionality of the API defined in
- drivers/connector/cn_hash.c.
- CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needs
- to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and
- CONFIG_KUNIT=m in .config file to compile and then test as a kernel
- module with "modprobe cn_hash_test".
- For more information on KUnit and unit tests in general please
- refer to the KUnit documentation in Documentation/dev-tools/kunit/.
- If unsure, say N.
Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the existing kunit tester complains about the missing symbols (see below). Please also hold off reposting for a couple of days to give people some time to review.
ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del': cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_display_htable': cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist' ld: vmlinux.o: in function `cn_hash_test_del_get_exval': cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_dup_add': cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval' ld: vmlinux.o: in function `cn_hash_test_del': cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_add': cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval' make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1 make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2 make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2 make: *** [Makefile:224: __sub-make] Error 2
Yes, I have added in the comments for CN_HASH_KUNIT_TEST, it depends on: CONFIG_CONNECTOR, CONFIG_PROC_EVENTS, CONFIG_NET. I didn’t realize I could add these to the “depends” field. So something like this: (let me know if you see any issues)
tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS depends on KUNIT
depends on CONNECTOR && PROC_EVENTSdepends on NET default KUNIT_ALL_TESTSThese are the configs I add to my .config file and compile it as a module and then do modprobe to test.
[..]
Are you running the kunit tester with kunit.py?
Yes, make sure all required options are picked up by "./tools/testing/kunit/kunit.py run" instead of manually adding options and doing modprobe.
I’m unable to run kunit.py, it runs into various issues like UML, permissions, other errors. I talked to the kunit guys about this and we have been debugging it for a while but unable to fix the environment issue. But the tests work fine.
What kind of VM is this being run on? Like ubuntu etc.? I will try on a different OS and check if kunit.py works.
Anjali
On 10/18, Anjali Kulkarni wrote:
On Oct 17, 2024, at 5:55 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/18, Anjali Kulkarni wrote:
On Oct 17, 2024, at 5:13 PM, Stanislav Fomichev stfomichev@gmail.com wrote:
On 10/17, Anjali Kulkarni wrote:
Kunit tests to test hash table add, delete, duplicate add and delete. Add following configs and compile kernel code:
CONFIG_CONNECTOR=y CONFIG_PROC_EVENTS=y CONFIG_NET=y CONFIG_KUNIT=m CONFIG_CN_HASH_KUNIT_TEST=m
To run kunit tests: sudo modprobe cn_hash_test
Output of kunit tests and hash table contents are displayed in /var/log/messages (at KERN_DEBUG level).
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
drivers/connector/cn_hash.c | 40 ++++++++ drivers/connector/connector.c | 12 +++ include/linux/connector.h | 4 + lib/Kconfig.debug | 17 ++++ lib/Makefile | 1 + lib/cn_hash_test.c | 167 ++++++++++++++++++++++++++++++++++ lib/cn_hash_test.h | 10 ++ 7 files changed, 251 insertions(+) create mode 100644 lib/cn_hash_test.c create mode 100644 lib/cn_hash_test.h
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c index a079e9bcea6d..40099b5908ac 100644 --- a/drivers/connector/cn_hash.c +++ b/drivers/connector/cn_hash.c @@ -170,6 +170,46 @@ int cn_hash_get_exval(struct cn_hash_dev *hdev, pid_t pid) return -EINVAL; }
+int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display)
+{
- struct uexit_pid_hnode *hnode;
- int key, count = 0;
- mutex_lock(&hdev->uexit_hash_lock);
- key = hash_min(pid, HASH_BITS(hdev->uexit_pid_htable));
- pr_debug("Bucket: %d\n", key);
- hlist_for_each_entry(hnode,
- &hdev->uexit_pid_htable[key],
- uexit_pid_hlist) {
- if (key_display[key] != 1) {
- if (hnode->uexit_pid_hlist.next == NULL)
- pr_debug("pid %d ", hnode->pid);
- else
- pr_debug("pid %d --> ", hnode->pid);
- }
- count++;
- }
- mutex_unlock(&hdev->uexit_hash_lock);
- if ((key_display[key] != 1) && !count)
- pr_debug("(empty)\n");
- pr_debug("\n");
- *hkey = key;
- if (count > max_len) {
- pr_err("%d entries in hlist for key %d, expected %d\n",
- count, key, max_len);
- return -EINVAL;
- }
- return 0;
+}
bool cn_hash_table_empty(struct cn_hash_dev *hdev) { bool is_empty; diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c index c1c0dcec53c0..2be2fe1adc12 100644 --- a/drivers/connector/connector.c +++ b/drivers/connector/connector.c @@ -304,6 +304,18 @@ int cn_get_exval(pid_t pid) } EXPORT_SYMBOL_GPL(cn_get_exval);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display) +{
- struct cn_dev *dev = &cdev;
- if (!cn_already_initialized)
- return 0;
- return cn_hash_display_hlist(dev->hdev, pid, max_len,
- hkey, key_display);
+} +EXPORT_SYMBOL_GPL(cn_display_hlist);
bool cn_table_empty(void) { struct cn_dev *dev = &cdev; diff --git a/include/linux/connector.h b/include/linux/connector.h index 5384e4bb98e8..a75c3fcf182a 100644 --- a/include/linux/connector.h +++ b/include/linux/connector.h @@ -168,4 +168,8 @@ int cn_get_exval(pid_t pid); bool cn_table_empty(void); bool cn_hash_table_empty(struct cn_hash_dev *hdev);
+int cn_display_hlist(pid_t pid, int max_len, int *hkey, int *key_display); +int cn_hash_display_hlist(struct cn_hash_dev *hdev, pid_t pid, int max_len,
- int *hkey, int *key_display);
#endif /* __CONNECTOR_H */ diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index 7315f643817a..290cf0a6befa 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2705,6 +2705,23 @@ config HASHTABLE_KUNIT_TEST
If unsure, say N.
+config CN_HASH_KUNIT_TEST
- tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS
- depends on KUNIT
- default KUNIT_ALL_TESTS
- help
- This builds the hashtable KUnit test suite.
- It tests the basic functionality of the API defined in
- drivers/connector/cn_hash.c.
- CONFIG_CONNECTOR=y, CONFIG_PROC_EVENTS=y and CONFIG_NET=y needs
- to be enabled along with CONFIG_CN_HASH_KUNIT_TEST=m and
- CONFIG_KUNIT=m in .config file to compile and then test as a kernel
- module with "modprobe cn_hash_test".
- For more information on KUnit and unit tests in general please
- refer to the KUnit documentation in Documentation/dev-tools/kunit/.
- If unsure, say N.
Looks like this needs to depend on CONFIG_CONNECTOR? Otherwise, the existing kunit tester complains about the missing symbols (see below). Please also hold off reposting for a couple of days to give people some time to review.
ERROR:root:ld: vmlinux.o: in function `cn_hash_test_dup_del': cn_hash_test.c:(.text+0x3e9dc3): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9dee): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3e9e22): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_display_htable': cn_hash_test.c:(.text+0x3e9f67): undefined reference to `cn_display_hlist' ld: vmlinux.o: in function `cn_hash_test_del_get_exval': cn_hash_test.c:(.text+0x3ea037): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea088): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_dup_add': cn_hash_test.c:(.text+0x3ea176): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea19e): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea1dc): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea205): undefined reference to `cn_get_exval' ld: vmlinux.o: in function `cn_hash_test_del': cn_hash_test.c:(.text+0x3ea387): undefined reference to `cn_del_get_exval' ld: cn_hash_test.c:(.text+0x3ea3ab): undefined reference to `cn_get_exval' ld: cn_hash_test.c:(.text+0x3ea3fd): undefined reference to `cn_table_empty' ld: vmlinux.o: in function `cn_hash_test_add': cn_hash_test.c:(.text+0x3ea571): undefined reference to `cn_add_elem' ld: cn_hash_test.c:(.text+0x3ea591): undefined reference to `cn_get_exval' make[3]: *** [../scripts/Makefile.vmlinux:34: vmlinux] Error 1 make[2]: *** [/home/kunit/testing/Makefile:1166: vmlinux] Error 2 make[1]: *** [/home/kunit/testing/Makefile:224: __sub-make] Error 2 make: *** [Makefile:224: __sub-make] Error 2
Yes, I have added in the comments for CN_HASH_KUNIT_TEST, it depends on: CONFIG_CONNECTOR, CONFIG_PROC_EVENTS, CONFIG_NET. I didn’t realize I could add these to the “depends” field. So something like this: (let me know if you see any issues)
tristate "KUnit Test for connector hashtable code" if !KUNIT_ALL_TESTS depends on KUNIT
depends on CONNECTOR && PROC_EVENTSdepends on NET default KUNIT_ALL_TESTSThese are the configs I add to my .config file and compile it as a module and then do modprobe to test.
[..]
Are you running the kunit tester with kunit.py?
Yes, make sure all required options are picked up by "./tools/testing/kunit/kunit.py run" instead of manually adding options and doing modprobe.
I’m unable to run kunit.py, it runs into various issues like UML, permissions, other errors. I talked to the kunit guys about this and we have been debugging it for a while but unable to fix the environment issue. But the tests work fine.
What kind of VM is this being run on? Like ubuntu etc.? I will try on a different OS and check if kunit.py works.
It's running on fedora.
Hi Anjali,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Anjali-Kulkarni/connector-cn_... base: net-next/main patch link: https://lore.kernel.org/r/20241017181436.2047508-3-anjali.k.kulkarni%40oracl... patch subject: [PATCH net-next v5 2/3] connector/cn_proc: Kunit tests for threads hash table config: sparc-randconfig-001-20241019 (https://download.01.org/0day-ci/archive/20241019/202410190945.sGeQPUMr-lkp@i...) compiler: sparc-linux-gcc (GCC) 14.1.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410190945.sGeQPUMr-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202410190945.sGeQPUMr-lkp@intel.com/
All errors (new ones prefixed by >>):
sparc-linux-ld: lib/cn_hash_test.o: in function `cn_hash_test_dup_del':
lib/cn_hash_test.c:140:(.text+0x44): undefined reference to `cn_del_get_exval' sparc-linux-ld: lib/cn_hash_test.c:143:(.text+0xb4): undefined reference to `cn_del_get_exval' sparc-linux-ld: lib/cn_hash_test.c:146:(.text+0x128): undefined reference to `cn_table_empty'
sparc-linux-ld: lib/cn_hash_test.o: in function `cn_display_htable':
lib/cn_hash_test.c:42:(.text+0x1f8): undefined reference to `cn_display_hlist'
sparc-linux-ld: lib/cn_hash_test.o: in function `cn_hash_test_del_get_exval': lib/cn_hash_test.c:103:(.text+0x2bc): undefined reference to `cn_del_get_exval' sparc-linux-ld: lib/cn_hash_test.c:109:(.text+0x350): undefined reference to `cn_table_empty' sparc-linux-ld: lib/cn_hash_test.o: in function `cn_hash_test_dup_add':
lib/cn_hash_test.c:118:(.text+0x3f0): undefined reference to `cn_add_elem' sparc-linux-ld: lib/cn_hash_test.c:121:(.text+0x458): undefined reference to `cn_get_exval' sparc-linux-ld: lib/cn_hash_test.c:127:(.text+0x4d8): undefined reference to `cn_add_elem'
sparc-linux-ld: lib/cn_hash_test.c:130:(.text+0x540): undefined reference to `cn_get_exval' sparc-linux-ld: lib/cn_hash_test.o: in function `cn_hash_test_del': lib/cn_hash_test.c:85:(.text+0x5d0): undefined reference to `cn_del_get_exval' sparc-linux-ld: lib/cn_hash_test.c:90:(.text+0x640): undefined reference to `cn_get_exval' sparc-linux-ld: lib/cn_hash_test.c:95:(.text+0x6cc): undefined reference to `cn_table_empty' sparc-linux-ld: lib/cn_hash_test.o: in function `cn_hash_test_add': lib/cn_hash_test.c:67:(.text+0x7b4): undefined reference to `cn_add_elem' sparc-linux-ld: lib/cn_hash_test.c:72:(.text+0x824): undefined reference to `cn_get_exval'
Kconfig warnings: (for reference only) WARNING: unmet direct dependencies detected for GET_FREE_REGION Depends on [n]: SPARSEMEM [=n] Selected by [y]: - RESOURCE_KUNIT_TEST [=y] && RUNTIME_TESTING_MENU [=y] && KUNIT [=y]
vim +140 lib/cn_hash_test.c
31 32 static void cn_display_htable(struct kunit *test, int len) 33 { 34 int i, err; 35 36 cn_hash_init(test); 37 38 pr_debug("\n"); 39 pr_debug("Displaying hash table:\n"); 40 41 for (i = 0; i < len; i++) {
42 err = cn_display_hlist(adata[i].pid, len, &adata[i].key,
43 key_display); 44 key_display[adata[i].key] = 1; 45 KUNIT_EXPECT_EQ(test, err, 0); 46 } 47 } 48 49 static void cn_hash_test_add(struct kunit *test) 50 { 51 int err, i; 52 int exit_val; 53 54 adata[0].pid = 1; 55 adata[0].exit_val = 45; 56 57 adata[1].pid = 2; 58 adata[1].exit_val = 13; 59 60 adata[2].pid = 1024; 61 adata[2].exit_val = 16; 62 63 adata[3].pid = 1023; 64 adata[3].exit_val = 71; 65 66 for (i = 0; i < ARRAY_SIZE(adata); i++) { 67 err = cn_add_elem(adata[i].exit_val, adata[i].pid); 68 KUNIT_EXPECT_EQ_MSG(test, 0, err, 69 "Adding pid %d returned err %d", 70 adata[i].pid, err); 71 72 exit_val = cn_get_exval(adata[i].pid); 73 KUNIT_EXPECT_EQ(test, adata[i].exit_val, exit_val); 74 } 75 76 cn_display_htable(test, ARRAY_SIZE(adata)); 77 } 78 79 static void cn_hash_test_del(struct kunit *test) 80 { 81 int i, err; 82 int exit_val; 83 84 for (i = 0; i < ARRAY_SIZE(adata); i++) { 85 err = cn_del_get_exval(adata[i].pid); 86 KUNIT_EXPECT_GT_MSG(test, err, 0, 87 "Deleting pid %d returned err %d", 88 adata[i].pid, err); 89 90 exit_val = cn_get_exval(adata[i].pid); 91 KUNIT_EXPECT_EQ(test, -EINVAL, exit_val); 92 } 93 94 cn_display_htable(test, ARRAY_SIZE(adata)); 95 KUNIT_EXPECT_TRUE(test, cn_table_empty()); 96 } 97 98 static void cn_hash_test_del_get_exval(struct kunit *test) 99 { 100 int i, exval; 101 102 for (i = 0; i < ARRAY_SIZE(adata); i++) { 103 exval = cn_del_get_exval(adata[i].pid); 104 KUNIT_EXPECT_EQ(test, adata[i].exit_val, exval); 105 106 cn_display_htable(test, ARRAY_SIZE(adata)); 107 } 108 109 KUNIT_EXPECT_TRUE(test, cn_table_empty()); 110 } 111 static void cn_hash_test_dup_add(struct kunit *test) 112 { 113 int err, exit_val; 114 115 adata[0].pid = 10; 116 adata[0].exit_val = 21; 117
118 err = cn_add_elem(adata[0].exit_val, adata[0].pid);
119 KUNIT_EXPECT_EQ(test, 0, err); 120
121 exit_val = cn_get_exval(adata[0].pid);
122 KUNIT_EXPECT_EQ(test, 21, exit_val); 123 124 adata[1].pid = 10; 125 adata[1].exit_val = 12; 126
127 err = cn_add_elem(adata[1].exit_val, adata[1].pid);
128 KUNIT_EXPECT_EQ(test, -EEXIST, err); 129 130 exit_val = cn_get_exval(adata[1].pid); 131 KUNIT_EXPECT_EQ(test, 21, exit_val); 132 133 cn_display_htable(test, 1); 134 } 135 136 static void cn_hash_test_dup_del(struct kunit *test) 137 { 138 int err; 139
140 err = cn_del_get_exval(adata[0].pid);
141 KUNIT_EXPECT_EQ(test, adata[0].exit_val, err); 142
143 err = cn_del_get_exval(adata[0].pid);
144 KUNIT_EXPECT_EQ(test, -EINVAL, err); 145
146 KUNIT_EXPECT_TRUE(test, cn_table_empty());
147 } 148
Hi Anjali,
kernel test robot noticed the following build errors:
[auto build test ERROR on net-next/main]
url: https://github.com/intel-lab-lkp/linux/commits/Anjali-Kulkarni/connector-cn_... base: net-next/main patch link: https://lore.kernel.org/r/20241017181436.2047508-3-anjali.k.kulkarni%40oracl... patch subject: [PATCH net-next v5 2/3] connector/cn_proc: Kunit tests for threads hash table config: x86_64-randconfig-104-20241019 (https://download.01.org/0day-ci/archive/20241019/202410191051.p3iFT9om-lkp@i...) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241019/202410191051.p3iFT9om-lkp@i...)
If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot lkp@intel.com | Closes: https://lore.kernel.org/oe-kbuild-all/202410191051.p3iFT9om-lkp@intel.com/
All errors (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in mm/kasan/kasan_test.o WARNING: modpost: missing MODULE_DESCRIPTION() in lib/slub_kunit.o
ERROR: modpost: "cn_del_get_exval" [lib/cn_hash_test.ko] undefined! ERROR: modpost: "cn_table_empty" [lib/cn_hash_test.ko] undefined! ERROR: modpost: "cn_display_hlist" [lib/cn_hash_test.ko] undefined! ERROR: modpost: "cn_add_elem" [lib/cn_hash_test.ko] undefined! ERROR: modpost: "cn_get_exval" [lib/cn_hash_test.ko] undefined!
Test to check if setting PROC_CN_MCAST_NOTIFY in proc connector API, allows a thread's non-zero exit status to be returned to proc_filter.
The threads.c program creates 2 child threads. 1st thread handles signal SIGSEGV, and 2nd thread needs to indicate some error condition (value 1) to the kernel, instead of using pthread_exit() with 1.
In both cases, child sends notify_netlink_thread_exit(exit_code) to kernel, to let kernel know it has exited abnormally with exit_code.
Compile: make thread make proc_filter Run: ./threads
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com --- tools/testing/selftests/connector/Makefile | 23 +- .../testing/selftests/connector/proc_filter.c | 34 ++- tools/testing/selftests/connector/thread.c | 232 ++++++++++++++++++ .../selftests/connector/thread_filter.c | 96 ++++++++ 4 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 tools/testing/selftests/connector/thread.c create mode 100644 tools/testing/selftests/connector/thread_filter.c
diff --git a/tools/testing/selftests/connector/Makefile b/tools/testing/selftests/connector/Makefile index 92188b9bac5c..bf335826bc3b 100644 --- a/tools/testing/selftests/connector/Makefile +++ b/tools/testing/selftests/connector/Makefile @@ -1,5 +1,26 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -Wall $(KHDR_INCLUDES) +KERNEL="../../../.." + +CFLAGS += -Wall $(KHDR_INCLUDES) -I $(KERNEL)/include/uapi -I $(KERNEL)/include + +proc_filter: proc_filter.o + cc proc_filter.o -o proc_filter + +proc_filter.o: proc_filter.c + cc -c proc_filter.c -o proc_filter.o $(CFLAGS) + +thread: thread.o thread_filter.o + cc thread.o thread_filter.o -o thread + +thread.o: thread.c $(DEPS) + cc -c thread.c -o thread.o $(CFLAGS) + +thread_filter.o: thread_filter.c + cc -c thread_filter.c -o thread_filter.o $(CFLAGS) + +define EXTRA_CLEAN + rm *.o thread +endef
TEST_GEN_PROGS = proc_filter
diff --git a/tools/testing/selftests/connector/proc_filter.c b/tools/testing/selftests/connector/proc_filter.c index 4a825b997666..5bf992deb792 100644 --- a/tools/testing/selftests/connector/proc_filter.c +++ b/tools/testing/selftests/connector/proc_filter.c @@ -1,4 +1,9 @@ // SPDX-License-Identifier: GPL-2.0-only +/* + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + * + * Copyright (c) 2024 Oracle and/or its affiliates. + */
#include <sys/types.h> #include <sys/epoll.h> @@ -28,6 +33,7 @@ volatile static int interrupted; static int nl_sock, ret_errno, tcount; static struct epoll_event evn; +FILE *file;
static int filter;
@@ -37,6 +43,8 @@ static int filter; #define Printf ksft_print_msg #endif
+#define EXIT_LOG + int send_message(void *pinp) { char buff[NL_MESSAGE_SIZE]; @@ -146,6 +154,12 @@ int handle_packet(char *buff, int fd, struct proc_event *event) tcount++; switch (event->what) { case PROC_EVENT_EXIT: +#ifdef EXIT_LOG + fprintf(file, "pid %d tgid %d code %d\n", + event->event_data.exit.process_pid, + event->event_data.exit.process_tgid, + event->event_data.exit.exit_code); +#endif Printf("Exit process %d (tgid %d) with code %d, signal %d\n", event->event_data.exit.process_pid, event->event_data.exit.process_tgid, @@ -279,17 +293,24 @@ int main(int argc, char *argv[]) exit(1); }
+#ifdef EXIT_LOG + file = fopen("exit.log", "w"); + if (file == NULL) { + perror("Error opening file exit.log"); + close(nl_sock); + close(epoll_fd); + exit(1); + } +#endif + while (!interrupted) { err = handle_events(epoll_fd, &proc_ev); if (err < 0) { if (ret_errno == EINTR) continue; - if (err == -2) - close(nl_sock); - if (err == -3) { - close(nl_sock); - close(epoll_fd); - } + close(nl_sock); + close(epoll_fd); + fclose(file); exit(1); } } @@ -304,6 +325,7 @@ int main(int argc, char *argv[])
close(epoll_fd); close(nl_sock); + fclose(file);
printf("Done total count: %d\n", tcount); exit(0); diff --git a/tools/testing/selftests/connector/thread.c b/tools/testing/selftests/connector/thread.c new file mode 100644 index 000000000000..70c475ce3176 --- /dev/null +++ b/tools/testing/selftests/connector/thread.c @@ -0,0 +1,232 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + * + * Copyright (c) 2024 Oracle and/or its affiliates. + */ + +#include <pthread.h> +#include <stdio.h> +#include <unistd.h> +#include <stdlib.h> +#include <signal.h> + +/* + * This code tests a thread exit notification when thread exits abnormally. + * Normally, when a thread exits abnormally, the kernel is not aware of the + * exit code. This is usually only conveyed from child to parent via the + * pthread_exit() and pthread_join() calls. Sometimes, however, a parent + * process cannot monitor all child processes via pthread_join(), particularly + * when there is a huge amount of child processes. In this case, the parent + * has created the child with PTHREAD_CREATE_DETACHED attribute. + * To fix this problem, either when child wants to convey non-zero exit via + * pthread_exit() or in a signal handler, the child can notify the kernel's + * connector module it's exit status via a netlink call with new type + * PROC_CN_MCAST_NOTIFY. (Implemented in the thread_filter.c file). + * This will send the exit code from the child to the kernel, which the kernel + * can later return to proc_filter program when the child actually exits. + * Compile: + * make thread + * make proc_filter + * Run: + * ./threads + */ + +extern int notify_netlink_thread_exit(unsigned int exit_code); + +static void sigsegvh(int sig) +{ + unsigned int exit_code = (unsigned int) sig; + /* + * Send any non-zero value to get a notification. Here we are + * sending the signal number for SIGSEGV which is 11 + */ + notify_netlink_thread_exit(exit_code); +} + +void *threadc1(void *ptr) +{ + signal(SIGSEGV, sigsegvh); + + *(int *)ptr = gettid(); + + printf("Child 1 thread id %d, handling SIGSEGV\n", gettid()); + sleep(10); + pthread_exit(NULL); +} + +void *threadc2(void *ptr) +{ + int exit_val = 1; + + *(int *)ptr = gettid(); + + printf("Child 2 thread id %d, wants to exit with value %d\n", + gettid(), exit_val); + sleep(2); + notify_netlink_thread_exit(exit_val); + pthread_exit(NULL); +} + +static void verify_exit_status(int tid1, int tid2) +{ + int found1 = 0, found2 = 0; + int pid, tgid, exit_code; + size_t size = 1024; + FILE *file; + char *data; + int ret; + + data = malloc(size * sizeof(char)); + if (data == NULL) { + perror("malloc for data failed"); + exit(1); + } + + file = fopen("exit.log", "r"); + if (file == NULL) { + perror("fopen of exit.log failed"); + free(data); + exit(1); + } + + while (getline(&data, &size, file) != -1) { + ret = sscanf(data, "pid %d tgid %d code %d", + &pid, &tgid, &exit_code); + if (ret != 3) { + perror("sscanf error"); + free(data); + fclose(file); + exit(1); + } + + if (tgid != getpid()) + continue; + + if (pid == tid1) { + if (exit_code == 11) { + printf("Successful notification of SIGSEGV, tid %d\n", + pid); + } else { + printf("Failure SIGSEGV tid %d, exit code %d\n", + pid, exit_code); + } + found1 = 1; + } else if (pid == tid2) { + if (exit_code == 1) { + printf("Successful notification of thread exit tid %d\n", + pid); + } else { + printf("Failure thread exit tid %d, exit code %d\n", + pid, exit_code); + } + found2 = 1; + } + } + + if (!found1) + printf("tid %d not present in exit.log file\n", tid1); + + if (!found2) + printf("tid %d not present in exit.log file\n", tid2); + + fclose(file); + free(data); +} + +static inline void init_threads(pthread_attr_t *attr) +{ + int ret; + + ret = pthread_attr_init(attr); + if (ret != 0) { + perror("pthread_attr_init failed"); + exit(ret); + } + + ret = pthread_attr_setdetachstate(attr, PTHREAD_CREATE_DETACHED); + if (ret != 0) { + perror("pthread_attr_setdetachstate failed"); + exit(ret); + } +} + +static inline void destroy_thread_attr(pthread_attr_t *attr) +{ + int ret; + + ret = pthread_attr_destroy(attr); + if (ret != 0) { + perror("pthread_attr_destroy failed"); + exit(ret); + } +} + + +static inline pid_t start_proc_filter(void) +{ + pid_t proc_filter = 0; + + proc_filter = fork(); + if (proc_filter == -1) { + perror("fork()"); + exit(1); + } + + if (proc_filter == 0) { + char* arr[] = {"proc_filter", "-f", NULL}; + execv("./proc_filter", arr); + } + sleep(1); + return proc_filter; +} + +int main(int argc, char **argv) +{ + pthread_t thread1, thread2; + pthread_attr_t attr1, attr2; + int tid1, tid2, ret; + pid_t proc_filter_pid; + + proc_filter_pid = start_proc_filter(); + + init_threads(&attr1); + ret = pthread_create(&thread1, &attr1, *threadc1, &tid1); + if (ret != 0) { + perror("pthread_create failed"); + exit(ret); + } + + init_threads(&attr2); + ret = pthread_create(&thread2, &attr2, *threadc2, &tid2); + if (ret != 0) { + perror("pthread_create failed"); + exit(ret); + } + + sleep(1); + + /* Send SIGSEGV to tid1 */ + kill(tid1, SIGSEGV); + + /* + * Wait for children to exit or be killed and for exit.log to + * be generated by ./proc_filter + */ + sleep(2); + + /* + * Kill proc_filter to get exit.log + */ + kill(proc_filter_pid, SIGINT); + + /* Required to allow kill to be processed */ + sleep(1); + + verify_exit_status(tid1, tid2); + + destroy_thread_attr(&attr1); + destroy_thread_attr(&attr2); + + exit(0); +} diff --git a/tools/testing/selftests/connector/thread_filter.c b/tools/testing/selftests/connector/thread_filter.c new file mode 100644 index 000000000000..3da740aa7537 --- /dev/null +++ b/tools/testing/selftests/connector/thread_filter.c @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Author: Anjali Kulkarni anjali.k.kulkarni@oracle.com + * + * Copyright (c) 2024 Oracle and/or its affiliates. + */ + +#include <sys/types.h> +#include <sys/epoll.h> +#include <sys/socket.h> +#include <linux/netlink.h> +#include <linux/connector.h> +#include <linux/cn_proc.h> + +#include <stddef.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include <strings.h> +#include <errno.h> +#include <signal.h> +#include <string.h> + +#define NL_MESSAGE_SIZE (sizeof(struct nlmsghdr) + sizeof(struct cn_msg) + \ + sizeof(struct proc_input)) + +/* + * Send PROC_CN_MCAST_NOTIFY type notification to the connector code in kernel. + * This will send the exit_code specified by user to the connector layer, so + * it can send a notification for that event to any listening process + */ +int send_message(int nl_sock, unsigned int exit_code) +{ + char buff[NL_MESSAGE_SIZE]; + struct nlmsghdr *hdr; + struct cn_msg *msg; + + hdr = (struct nlmsghdr *)buff; + hdr->nlmsg_len = NL_MESSAGE_SIZE; + hdr->nlmsg_type = NLMSG_DONE; + hdr->nlmsg_flags = 0; + hdr->nlmsg_seq = 0; + hdr->nlmsg_pid = getpid(); + + msg = (struct cn_msg *)NLMSG_DATA(hdr); + msg->id.idx = CN_IDX_PROC; + msg->id.val = CN_VAL_PROC; + msg->seq = 0; + msg->ack = 0; + msg->flags = 0; + + msg->len = sizeof(struct proc_input); + ((struct proc_input *)msg->data)->mcast_op = + PROC_CN_MCAST_NOTIFY; + ((struct proc_input *)msg->data)->uexit_code = exit_code; + + if (send(nl_sock, hdr, hdr->nlmsg_len, 0) == -1) { + perror("send failed"); + return -errno; + } + return 0; +} + +int notify_netlink_thread_exit(unsigned int exit_code) +{ + struct sockaddr_nl sa_nl; + int err = 0; + int nl_sock; + + nl_sock = socket(PF_NETLINK, SOCK_DGRAM, NETLINK_CONNECTOR); + + if (nl_sock == -1) { + perror("socket failed"); + return -errno; + } + + bzero(&sa_nl, sizeof(sa_nl)); + sa_nl.nl_family = AF_NETLINK; + sa_nl.nl_groups = CN_IDX_PROC; + sa_nl.nl_pid = gettid(); + + if (bind(nl_sock, (struct sockaddr *)&sa_nl, sizeof(sa_nl)) == -1) { + perror("bind failed"); + close(nl_sock); + return -errno; + } + + err = send_message(nl_sock, exit_code); + + close(nl_sock); + + if (err < 0) + return err; + + return 0; +}
On Thu, Oct 17, 2024 at 11:14:36AM -0700, Anjali Kulkarni wrote:
Test to check if setting PROC_CN_MCAST_NOTIFY in proc connector API, allows a thread's non-zero exit status to be returned to proc_filter.
The threads.c program creates 2 child threads. 1st thread handles signal SIGSEGV, and 2nd thread needs to indicate some error condition (value 1) to the kernel, instead of using pthread_exit() with 1.
In both cases, child sends notify_netlink_thread_exit(exit_code) to kernel, to let kernel know it has exited abnormally with exit_code.
Compile: make thread make proc_filter Run: ./threads
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
tools/testing/selftests/connector/Makefile | 23 +- .../testing/selftests/connector/proc_filter.c | 34 ++- tools/testing/selftests/connector/thread.c | 232 ++++++++++++++++++ .../selftests/connector/thread_filter.c | 96 ++++++++ 4 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 tools/testing/selftests/connector/thread.c create mode 100644 tools/testing/selftests/connector/thread_filter.c
diff --git a/tools/testing/selftests/connector/Makefile b/tools/testing/selftests/connector/Makefile index 92188b9bac5c..bf335826bc3b 100644 --- a/tools/testing/selftests/connector/Makefile +++ b/tools/testing/selftests/connector/Makefile @@ -1,5 +1,26 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -Wall $(KHDR_INCLUDES) +KERNEL="../../../.."
+CFLAGS += -Wall $(KHDR_INCLUDES) -I $(KERNEL)/include/uapi -I $(KERNEL)/include
+proc_filter: proc_filter.o
- cc proc_filter.o -o proc_filter
+proc_filter.o: proc_filter.c
- cc -c proc_filter.c -o proc_filter.o $(CFLAGS)
+thread: thread.o thread_filter.o
- cc thread.o thread_filter.o -o thread
+thread.o: thread.c $(DEPS)
cc -c thread.c -o thread.o $(CFLAGS)+thread_filter.o: thread_filter.c
cc -c thread_filter.c -o thread_filter.o $(CFLAGS)+define EXTRA_CLEAN
- rm *.o thread
+endef TEST_GEN_PROGS = proc_filter
I am a little confused by this, as it seems to result in user-space code using kernel headers. Is that expected?
$ make -C tools/testing/selftests/connector ... cc -c proc_filter.c -o proc_filter.o -Wall -isystem /home/horms/projects/linux/linux/tools/testing/selftests/../../../usr/include -I "../../../.."/include/uapi -I "../../../.."/include -D_GNU_SOURCE= In file included from ../../../../include/uapi/linux/netlink.h:7, from proc_filter.c:11: ../../../../include/uapi/linux/types.h:10:2: warning: #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders" [-Wcpp] 10 | #warning "Attempt to use kernel headers from user space, see https://kernelnewbies.org/KernelHeaders" | ^~~~~~~ ...
diff --git a/tools/testing/selftests/connector/thread.c b/tools/testing/selftests/connector/thread.c
...
+static inline void init_threads(pthread_attr_t *attr)
Please don't use inline in .c files unless there is a demonstrable, usually performance, reason to do so.
Likewise twice more in this patch and once in patch 1/3.
+{
- int ret;
- ret = pthread_attr_init(attr);
- if (ret != 0) {
perror("pthread_attr_init failed");exit(ret);- }
- ret = pthread_attr_setdetachstate(attr, PTHREAD_CREATE_DETACHED);
- if (ret != 0) {
perror("pthread_attr_setdetachstate failed");exit(ret);- }
+}
...
On Oct 18, 2024, at 3:04 AM, Simon Horman horms@kernel.org wrote:
On Thu, Oct 17, 2024 at 11:14:36AM -0700, Anjali Kulkarni wrote:
Test to check if setting PROC_CN_MCAST_NOTIFY in proc connector API, allows a thread's non-zero exit status to be returned to proc_filter.
The threads.c program creates 2 child threads. 1st thread handles signal SIGSEGV, and 2nd thread needs to indicate some error condition (value 1) to the kernel, instead of using pthread_exit() with 1.
In both cases, child sends notify_netlink_thread_exit(exit_code) to kernel, to let kernel know it has exited abnormally with exit_code.
Compile: make thread make proc_filter Run: ./threads
Signed-off-by: Anjali Kulkarni anjali.k.kulkarni@oracle.com
tools/testing/selftests/connector/Makefile | 23 +- .../testing/selftests/connector/proc_filter.c | 34 ++- tools/testing/selftests/connector/thread.c | 232 ++++++++++++++++++ .../selftests/connector/thread_filter.c | 96 ++++++++ 4 files changed, 378 insertions(+), 7 deletions(-) create mode 100644 tools/testing/selftests/connector/thread.c create mode 100644 tools/testing/selftests/connector/thread_filter.c
diff --git a/tools/testing/selftests/connector/Makefile b/tools/testing/selftests/connector/Makefile index 92188b9bac5c..bf335826bc3b 100644 --- a/tools/testing/selftests/connector/Makefile +++ b/tools/testing/selftests/connector/Makefile @@ -1,5 +1,26 @@ # SPDX-License-Identifier: GPL-2.0 -CFLAGS += -Wall $(KHDR_INCLUDES) +KERNEL="../../../.."
+CFLAGS += -Wall $(KHDR_INCLUDES) -I $(KERNEL)/include/uapi -I $(KERNEL)/include
+proc_filter: proc_filter.o
- cc proc_filter.o -o proc_filter
+proc_filter.o: proc_filter.c
- cc -c proc_filter.c -o proc_filter.o $(CFLAGS)
+thread: thread.o thread_filter.o
- cc thread.o thread_filter.o -o thread
+thread.o: thread.c $(DEPS)
- cc -c thread.c -o thread.o $(CFLAGS)
+thread_filter.o: thread_filter.c
- cc -c thread_filter.c -o thread_filter.o $(CFLAGS)
+define EXTRA_CLEAN
- rm *.o thread
+endef
TEST_GEN_PROGS = proc_filter
I am a little confused by this, as it seems to result in user-space code using kernel headers. Is that expected?
If I do not do this, then it’s not possible to run the selftests on a system which does not have the built image installed. This allows me to to test the selftest code in the build tree without having to install the build kernel on the system, as it uses the header files in the build itself instead of the ones installed on the system. I can remove it if required.
Anjali
$ make -C tools/testing/selftests/connector ... cc -c proc_filter.c -o proc_filter.o -Wall -isystem /home/horms/projects/linux/linux/tools/testing/selftests/../../../usr/include -I "../../../.."/include/uapi -I "../../../.."/include -D_GNU_SOURCE= In file included from ../../../../include/uapi/linux/netlink.h:7, from proc_filter.c:11: ../../../../include/uapi/linux/types.h:10:2: warning: #warning "Attempt to use kernel headers from user space, see https://urldefense.com/v3/__https://kernelnewbies.org/KernelHeaders__%3B%21%... " [-Wcpp] 10 | #warning "Attempt to use kernel headers from user space, see https://urldefense.com/v3/__https://kernelnewbies.org/KernelHeaders__%3B%21%... " | ^~~~~~~ ...
diff --git a/tools/testing/selftests/connector/thread.c b/tools/testing/selftests/connector/thread.c
...
+static inline void init_threads(pthread_attr_t *attr)
Please don't use inline in .c files unless there is a demonstrable, usually performance, reason to do so.
Likewise twice more in this patch and once in patch 1/3.
+{
- int ret;
- ret = pthread_attr_init(attr);
- if (ret != 0) {
- perror("pthread_attr_init failed");
- exit(ret);
- }
- ret = pthread_attr_setdetachstate(attr, PTHREAD_CREATE_DETACHED);
- if (ret != 0) {
- perror("pthread_attr_setdetachstate failed");
- exit(ret);
- }
+}
...
On Thu, Oct 17, 2024 at 11:14:33AM -0700, Anjali Kulkarni wrote:
Recently we committed a fix to allow processes to receive notifications for non-zero exits via the process connector module. Commit is a4c9a56e6a2c.
However, for threads, when it does a pthread_exit(&exit_status) call, the kernel is not aware of the exit status with which pthread_exit is called. It is sent by child thread to the parent process, if it is waiting in pthread_join(). Hence, for a thread exiting abnormally, kernel cannot send notifications to any listening processes.
The exception to this is if the thread is sent a signal which it has not handled, and dies along with it's process as a result; for eg. SIGSEGV or SIGKILL. In this case, kernel is aware of the non-zero exit and sends a notification for it.
For our use case, we cannot have parent wait in pthread_join, one of the main reasons for this being that we do not want to track normal pthread_exit(), which could be a very large number. We only want to be notified of any abnormal exits. Hence, threads are created with pthread_attr_t set to PTHREAD_CREATE_DETACHED.
To fix this problem, we add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a thread to send it's exit status to kernel either when it needs to call pthread_exit() with non-zero value to indicate some error or from signal handler before pthread_exit().
We also need to filter packets with non-zero exit notifications futher based on instances, which can be identified by task names. Hence, added a comm field to the packet's struct proc_event, in which task->comm is stored.
As it seems that there will be another revision anyway, please run this patch-set through checkpatch with the following arguments.
./scripts/checkpatch.pl --strict --max-line-length=80
And please fix warnings about alignment and line length. But please do so in such a way that doesn't reduce readability, e.g. don't split strings over multiple lines.
On Oct 18, 2024, at 2:49 AM, Simon Horman horms@kernel.org wrote:
On Thu, Oct 17, 2024 at 11:14:33AM -0700, Anjali Kulkarni wrote:
Recently we committed a fix to allow processes to receive notifications for non-zero exits via the process connector module. Commit is a4c9a56e6a2c.
However, for threads, when it does a pthread_exit(&exit_status) call, the kernel is not aware of the exit status with which pthread_exit is called. It is sent by child thread to the parent process, if it is waiting in pthread_join(). Hence, for a thread exiting abnormally, kernel cannot send notifications to any listening processes.
The exception to this is if the thread is sent a signal which it has not handled, and dies along with it's process as a result; for eg. SIGSEGV or SIGKILL. In this case, kernel is aware of the non-zero exit and sends a notification for it.
For our use case, we cannot have parent wait in pthread_join, one of the main reasons for this being that we do not want to track normal pthread_exit(), which could be a very large number. We only want to be notified of any abnormal exits. Hence, threads are created with pthread_attr_t set to PTHREAD_CREATE_DETACHED.
To fix this problem, we add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a thread to send it's exit status to kernel either when it needs to call pthread_exit() with non-zero value to indicate some error or from signal handler before pthread_exit().
We also need to filter packets with non-zero exit notifications futher based on instances, which can be identified by task names. Hence, added a comm field to the packet's struct proc_event, in which task->comm is stored.
As it seems that there will be another revision anyway, please run this patch-set through checkpatch with the following arguments.
./scripts/checkpatch.pl --strict --max-line-length=80
And please fix warnings about alignment and line length. But please do so in such a way that doesn't reduce readability, e.g. don't split strings over multiple lines.
Ok thanks, will do.
Anjali
linux-kselftest-mirror@lists.linaro.org
-
Anjali Kulkarni -
kernel test robot -
Simon Horman -
Stanislav Fomichev