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:
> > On Wed, Jun 09, 2021 at 11:07:08AM +0300, Dan Carpenter wrote:
> > > On Wed, Jun 09, 2021 at 10:31:29AM +0900, William Breathitt Gray wrote:
> > > > +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().
> > > > + /* 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
Attachment:
signature.asc
Description: PGP signature
