On Wed, Jun 09, 2021 at 11:59:38AM +0300, Dan Carpenter wrote:

On Wed, Jun 09, 2021 at 05:28:10PM +0900, William Breathitt Gray wrote:

quoted

On Wed, Jun 09, 2021 at 11:07:08AM +0300, Dan Carpenter wrote:

quoted

On Wed, Jun 09, 2021 at 10:31:29AM +0900, William Breathitt Gray wrote:

quoted

+static int counter_set_event_node(struct counter_device *const counter,
+				  struct counter_watch *const watch,
+				  const struct counter_comp_node *const cfg)
+{
+	struct counter_event_node *event_node;
+	struct counter_comp_node *comp_node;
+

The caller should be holding the counter->events_list_lock lock but it's
not.

Hi Dan,

The counter_set_event_node() function doesn't access or modify
counter->events_list (it works on counter->next_events_list) so holding
the counter->events_list_lock here isn't necessary.

There needs to be some sort of locking or this function can race with
itself.  (Two threads add the same event at exactly the same time).  It
looks like it can also race with counter_disable_events() leading to a
use after free.

All right, I'll add in a lock to protect this function so it doesn't
race with itself nor counter_disable_events().

quoted

quoted

quoted

+	/* Search for event in the list */
+	list_for_each_entry(event_node, &counter->next_events_list, l)
+		if (event_node->event == watch->event &&
+		    event_node->channel == watch->channel)
+			break;
+
+	/* If event is not already in the list */
+	if (&event_node->l == &counter->next_events_list) {
+		/* Allocate new event node */
+		event_node = kmalloc(sizeof(*event_node), GFP_ATOMIC);

Btw, say we decided that we can add/remove events locklessly, then these
GFP_ATOMICs can be changed to GFP_KERNEL.

Because I'll be using a lock I'll keep these as GFP_ATOMICs afterall.

Thanks,

William Breathitt Gray