Find user_events always while under the event_mutex and before leaving the lock, add a ref count to the user_event. This ensures that all paths under the event_mutex that check the ref counts will be synchronized. The ioctl add/delete paths are protected by the reg_mutex. However, dyn_event is only protected by the event_mutex. The dyn_event delete path cannot acquire reg_mutex, since that could cause a deadlock between the ioctl delete case acquiring event_mutex after acquiring the reg_mutex. Link: https://lkml.kernel.org/r/20220310001141.1660-1-beaub@linux.microsoft.com
Showing the changes to kernel (1 of 1 file). Show all
Unified · Split
| @@ -135,6 +135,8 @@ static struct list_head *user_event_get_fields(struct trace_event_call *call) · 134 unmodified lines | ||
| 135 | 135 | * NOTE: Offsets are from the user data perspective, they are not from the |
| 136 | 136 | * trace_entry/buffer perspective. We automatically add the common properties |
| 137 | 137 | * sizes to the offset for the user. |
| 138 | + * | |
| 139 | + * Upon success user_event has its ref count increased by 1. | |
| 138 | 140 | */ |
| 139 | 141 | static int user_event_parse_cmd(char *raw_command, struct user_event **newuser) |
| 140 | 142 | { |
| @@ -593,8 +595,10 @@ static struct user_event *find_user_event(char *name, u32 *outkey) · 452 unmodified lines | ||
| 593 | 595 | *outkey = key; |
| 594 | 596 | |
| 595 | 597 | hash_for_each_possible(register_table, user, node, key) |
| 596 | − if (!strcmp(EVENT_NAME(user), name)) | |
| 598 | + if (!strcmp(EVENT_NAME(user), name)) { | |
| 599 | + atomic_inc(&user->refcnt); | |
| 597 | 600 | return user; |
| 601 | + } | |
| 598 | 602 | |
| 599 | 603 | return NULL; |
| 600 | 604 | } |
| @@ -883,7 +887,12 @@ static int user_event_create(const char *raw_command) · 282 unmodified lines | ||
| 883 | 887 | return -ENOMEM; |
| 884 | 888 | |
| 885 | 889 | mutex_lock(®_mutex); |
| 890 | + | |
| 886 | 891 | ret = user_event_parse_cmd(name, &user); |
| 892 | + | |
| 893 | + if (!ret) | |
| 894 | + atomic_dec(&user->refcnt); | |
| 895 | + | |
| 887 | 896 | mutex_unlock(®_mutex); |
| 888 | 897 | |
| 889 | 898 | if (ret) |
| @@ -1050,6 +1059,7 @@ static int user_event_trace_register(struct user_event *user) · 160 unmodified lines | ||
| 1050 | 1059 | /* |
| 1051 | 1060 | * Parses the event name, arguments and flags then registers if successful. |
| 1052 | 1061 | * The name buffer lifetime is owned by this method for success cases only. |
| 1062 | + * Upon success the returned user_event has its ref count increased by 1. | |
| 1053 | 1063 | */ |
| 1054 | 1064 | static int user_event_parse(char *name, char *args, char *flags, |
| 1055 | 1065 | struct user_event **newuser) |
| @@ -1057,7 +1067,12 @@ static int user_event_parse(char *name, char *args, char *flags, · 1 unmodified line | ||
| 1057 | 1067 | int ret; |
| 1058 | 1068 | int index; |
| 1059 | 1069 | u32 key; |
| 1060 | − struct user_event *user = find_user_event(name, &key); | |
| 1070 | + struct user_event *user; | |
| 1071 | + | |
| 1072 | + /* Prevent dyn_event from racing */ | |
| 1073 | + mutex_lock(&event_mutex); | |
| 1074 | + user = find_user_event(name, &key); | |
| 1075 | + mutex_unlock(&event_mutex); | |
| 1061 | 1076 | |
| 1062 | 1077 | if (user) { |
| 1063 | 1078 | *newuser = user; |
| @@ -1121,6 +1136,10 @@ static int user_event_parse(char *name, char *args, char *flags, · 57 unmodified lines | ||
| 1121 | 1136 | goto put_user; |
| 1122 | 1137 | |
| 1123 | 1138 | user->index = index; |
| 1139 | + | |
| 1140 | + /* Ensure we track ref */ | |
| 1141 | + atomic_inc(&user->refcnt); | |
| 1142 | + | |
| 1124 | 1143 | dyn_event_init(&user->devent, &user_event_dops); |
| 1125 | 1144 | dyn_event_add(&user->devent, &user->call); |
| 1126 | 1145 | set_bit(user->index, page_bitmap); |
| @@ -1147,12 +1166,21 @@ static int delete_user_event(char *name) · 20 unmodified lines | ||
| 1147 | 1166 | if (!user) |
| 1148 | 1167 | return -ENOENT; |
| 1149 | 1168 | |
| 1150 | − if (atomic_read(&user->refcnt) != 0) | |
| 1151 | − return -EBUSY; | |
| 1169 | + /* Ensure we are the last ref */ | |
| 1170 | + if (atomic_read(&user->refcnt) != 1) { | |
| 1171 | + ret = -EBUSY; | |
| 1172 | + goto put_ref; | |
| 1173 | + } | |
| 1152 | 1174 | |
| 1153 | − mutex_lock(&event_mutex); | |
| 1154 | 1175 | ret = destroy_user_event(user); |
| 1155 | − mutex_unlock(&event_mutex); | |
| 1176 | + | |
| 1177 | + if (ret) | |
| 1178 | + goto put_ref; | |
| 1179 | + | |
| 1180 | + return ret; | |
| 1181 | +put_ref: | |
| 1182 | + /* No longer have this ref */ | |
| 1183 | + atomic_dec(&user->refcnt); | |
| 1156 | 1184 | |
| 1157 | 1185 | return ret; |
| 1158 | 1186 | } |
| @@ -1340,6 +1368,9 @@ static long user_events_ioctl_reg(struct file *file, unsigned long uarg) · 181 unmodified lines | ||
| 1340 | 1368 | |
| 1341 | 1369 | ret = user_events_ref_add(file, user); |
| 1342 | 1370 | |
| 1371 | + /* No longer need parse ref, ref_add either worked or not */ | |
| 1372 | + atomic_dec(&user->refcnt); | |
| 1373 | + | |
| 1343 | 1374 | /* Positive number is index and valid */ |
| 1344 | 1375 | if (ret < 0) |
| 1345 | 1376 | return ret; |
| @@ -1364,7 +1395,10 @@ static long user_events_ioctl_del(struct file *file, unsigned long uarg) · 18 unmodified lines | ||
| 1364 | 1395 | if (IS_ERR(name)) |
| 1365 | 1396 | return PTR_ERR(name); |
| 1366 | 1397 | |
| 1398 | + /* event_mutex prevents dyn_event from racing */ | |
| 1399 | + mutex_lock(&event_mutex); | |
| 1367 | 1400 | ret = delete_user_event(name); |
| 1401 | + mutex_unlock(&event_mutex); | |
| 1368 | 1402 | |
| 1369 | 1403 | kfree(name); |
| 1370 | 1404 | |