From 2230a7ef5198632bdbf844fcf0abdd7958a6ac7d Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:51 +0300 Subject: [PATCH 1/6] drop_monitor: Use correct error code The error code 'ENOTSUPP' is reserved for use with NFS. Use 'EOPNOTSUPP' instead. Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 4ea4347f5062..dcb4d2aeb2a8 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -298,7 +298,7 @@ out_unlock: static int net_dm_cmd_config(struct sk_buff *skb, struct genl_info *info) { - return -ENOTSUPP; + return -EOPNOTSUPP; } static int net_dm_cmd_trace(struct sk_buff *skb, @@ -311,7 +311,7 @@ static int net_dm_cmd_trace(struct sk_buff *skb, return set_all_monitor_traces(TRACE_OFF); } - return -ENOTSUPP; + return -EOPNOTSUPP; } static int dropmon_net_event(struct notifier_block *ev_block, From dbf896b70d4a4bdbd10aec060af42d7c70e6a88d Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:52 +0300 Subject: [PATCH 2/6] drop_monitor: Rename and document scope of mutex The 'trace_state_mutex' does not only protect the global 'trace_state' variable, but also the global 'hw_stats_list'. Subsequent patches are going add more operations from user space to drop_monitor and these all need to be mutually exclusive. Rename 'trace_state_mutex' to the more fitting 'net_dm_mutex' name and document its scope. Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index dcb4d2aeb2a8..000ec8b66d1e 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -43,7 +43,13 @@ * netlink alerts */ static int trace_state = TRACE_OFF; -static DEFINE_MUTEX(trace_state_mutex); + +/* net_dm_mutex + * + * An overall lock guarding every operation coming from userspace. + * It also guards the global 'hw_stats_list' list. + */ +static DEFINE_MUTEX(net_dm_mutex); struct per_cpu_dm_data { spinlock_t lock; @@ -241,7 +247,7 @@ static int set_all_monitor_traces(int state) struct dm_hw_stat_delta *new_stat = NULL; struct dm_hw_stat_delta *temp; - mutex_lock(&trace_state_mutex); + mutex_lock(&net_dm_mutex); if (state == trace_state) { rc = -EAGAIN; @@ -289,7 +295,7 @@ static int set_all_monitor_traces(int state) rc = -EINPROGRESS; out_unlock: - mutex_unlock(&trace_state_mutex); + mutex_unlock(&net_dm_mutex); return rc; } @@ -330,12 +336,12 @@ static int dropmon_net_event(struct notifier_block *ev_block, new_stat->dev = dev; new_stat->last_rx = jiffies; - mutex_lock(&trace_state_mutex); + mutex_lock(&net_dm_mutex); list_add_rcu(&new_stat->list, &hw_stats_list); - mutex_unlock(&trace_state_mutex); + mutex_unlock(&net_dm_mutex); break; case NETDEV_UNREGISTER: - mutex_lock(&trace_state_mutex); + mutex_lock(&net_dm_mutex); list_for_each_entry_safe(new_stat, tmp, &hw_stats_list, list) { if (new_stat->dev == dev) { new_stat->dev = NULL; @@ -346,7 +352,7 @@ static int dropmon_net_event(struct notifier_block *ev_block, } } } - mutex_unlock(&trace_state_mutex); + mutex_unlock(&net_dm_mutex); break; } out: From 01921d53f870653f04ebf8d3c375029ee3ca4a31 Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:53 +0300 Subject: [PATCH 3/6] drop_monitor: Document scope of spinlock While 'per_cpu_dm_data' is a per-CPU variable, its 'skb' and 'send_timer' fields can be accessed concurrently by the CPU sending the netlink notification to user space from the workqueue and the CPU tracing kfree_skb(). This spinlock is meant to protect against that. Document its scope and suppress the checkpatch message "spinlock_t definition without comment". Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 000ec8b66d1e..35d31b007da4 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -52,7 +52,7 @@ static int trace_state = TRACE_OFF; static DEFINE_MUTEX(net_dm_mutex); struct per_cpu_dm_data { - spinlock_t lock; + spinlock_t lock; /* Protects 'skb' and 'send_timer' */ struct sk_buff *skb; struct work_struct dm_alert_work; struct timer_list send_timer; From ff3818ca39c9f9ce07c4d50db594b9673dfa422c Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:54 +0300 Subject: [PATCH 4/6] drop_monitor: Avoid multiple blank lines Remove multiple blank lines which are visually annoying and useless. This suppresses the "Please don't use multiple blank lines" checkpatch messages. Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 35d31b007da4..9080e62245b9 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -300,7 +300,6 @@ out_unlock: return rc; } - static int net_dm_cmd_config(struct sk_buff *skb, struct genl_info *info) { @@ -427,7 +426,6 @@ static int __init init_net_drop_monitor(void) reset_per_cpu_data(data); } - goto out; out_unreg: From 965100966efe85e636178166fbf006e9b74f78d4 Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:55 +0300 Subject: [PATCH 5/6] drop_monitor: Add extack support Add various extack messages to make drop_monitor more user friendly. Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 9080e62245b9..1d463c0d4bc5 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -241,7 +241,7 @@ static void trace_napi_poll_hit(void *ignore, struct napi_struct *napi, rcu_read_unlock(); } -static int set_all_monitor_traces(int state) +static int set_all_monitor_traces(int state, struct netlink_ext_ack *extack) { int rc = 0; struct dm_hw_stat_delta *new_stat = NULL; @@ -250,6 +250,7 @@ static int set_all_monitor_traces(int state) mutex_lock(&net_dm_mutex); if (state == trace_state) { + NL_SET_ERR_MSG_MOD(extack, "Trace state already set to requested state"); rc = -EAGAIN; goto out_unlock; } @@ -257,6 +258,7 @@ static int set_all_monitor_traces(int state) switch (state) { case TRACE_ON: if (!try_module_get(THIS_MODULE)) { + NL_SET_ERR_MSG_MOD(extack, "Failed to take reference on module"); rc = -ENODEV; break; } @@ -303,6 +305,8 @@ out_unlock: static int net_dm_cmd_config(struct sk_buff *skb, struct genl_info *info) { + NL_SET_ERR_MSG_MOD(info->extack, "Command not supported"); + return -EOPNOTSUPP; } @@ -311,9 +315,9 @@ static int net_dm_cmd_trace(struct sk_buff *skb, { switch (info->genlhdr->cmd) { case NET_DM_CMD_START: - return set_all_monitor_traces(TRACE_ON); + return set_all_monitor_traces(TRACE_ON, info->extack); case NET_DM_CMD_STOP: - return set_all_monitor_traces(TRACE_OFF); + return set_all_monitor_traces(TRACE_OFF, info->extack); } return -EOPNOTSUPP; From b19d955055480ac4e03f5afec0ca80f0de7b7013 Mon Sep 17 00:00:00 2001 From: Ido Schimmel Date: Tue, 6 Aug 2019 16:19:56 +0300 Subject: [PATCH 6/6] drop_monitor: Use pre_doit / post_doit hooks Each operation from user space should be protected by the global drop monitor mutex. Use the pre_doit / post_doit hooks to take / release the lock instead of doing it explicitly in each function. Signed-off-by: Ido Schimmel Signed-off-by: David S. Miller --- net/core/drop_monitor.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c index 1d463c0d4bc5..4deb86f990f1 100644 --- a/net/core/drop_monitor.c +++ b/net/core/drop_monitor.c @@ -247,12 +247,9 @@ static int set_all_monitor_traces(int state, struct netlink_ext_ack *extack) struct dm_hw_stat_delta *new_stat = NULL; struct dm_hw_stat_delta *temp; - mutex_lock(&net_dm_mutex); - if (state == trace_state) { NL_SET_ERR_MSG_MOD(extack, "Trace state already set to requested state"); - rc = -EAGAIN; - goto out_unlock; + return -EAGAIN; } switch (state) { @@ -296,9 +293,6 @@ static int set_all_monitor_traces(int state, struct netlink_ext_ack *extack) else rc = -EINPROGRESS; -out_unlock: - mutex_unlock(&net_dm_mutex); - return rc; } @@ -380,10 +374,26 @@ static const struct genl_ops dropmon_ops[] = { }, }; +static int net_dm_nl_pre_doit(const struct genl_ops *ops, + struct sk_buff *skb, struct genl_info *info) +{ + mutex_lock(&net_dm_mutex); + + return 0; +} + +static void net_dm_nl_post_doit(const struct genl_ops *ops, + struct sk_buff *skb, struct genl_info *info) +{ + mutex_unlock(&net_dm_mutex); +} + static struct genl_family net_drop_monitor_family __ro_after_init = { .hdrsize = 0, .name = "NET_DM", .version = 2, + .pre_doit = net_dm_nl_pre_doit, + .post_doit = net_dm_nl_post_doit, .module = THIS_MODULE, .ops = dropmon_ops, .n_ops = ARRAY_SIZE(dropmon_ops),