> > +static void *hwspinlock_seq_start(struct seq_file *s, loff_t *ppos)
> > +{
> > + struct hwspinlock_seq_iterator *hwsp_seq_iter =
> > kzalloc(sizeof(*hwsp_seq_iter), GFP_KERNEL);
> > +
> > + seq_puts(s, "id\tstatus\tdevice\n");
>
> [Severity: Medium]
> Will this unconditionally print the table header on every chunked read?
>
> Because the seq_file framework calls start() at the beginning of every read()
> syscall or when reallocating an internal buffer, this header might be
> redundantly injected into the middle of the output.
>
> Should it be printed only when *ppos == 0, or handled in show() using
> SEQ_START_TOKEN?
Yes, I will do the latter.
> > +
> > + if (!hwsp_seq_iter)
> > + return NULL;
>
> [Severity: Medium]
> Should this return ERR_PTR(-ENOMEM) instead of NULL?
Yes.
> [Severity: High]
> Can this early return cause an unbalanced mutex unlock?
Yes.
> > +static void *hwspinlock_seq_next(struct seq_file *s, void *v, loff_t *ppos)
> > +{
> > + struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
> > +
> > + ++*ppos;
>
> [Severity: High]
> Does sequentially incrementing *ppos cause infinite loops or duplicate entries
> on chunked reads?
>
> In hwspinlock_seq_start(), *ppos is passed to radix_tree_iter_init() to
> search the sparse radix tree. However, *ppos is incremented sequentially
> here.
>
> Because registered hwspinlock IDs (the radix tree keys) are often sparse,
> *ppos quickly desynchronizes from the actual keys. If a read() operation
> resumes, seq_start() might search the radix tree starting from a sequential
> *ppos value much smaller than the actual next key, re-printing already output
> keys.
Probably true, will test.
>
> > +
> > + hwsp_seq_iter->slot = radix_tree_next_slot(hwsp_seq_iter->slot,
> > &hwsp_seq_iter->iter, 0);
> > + if (!hwsp_seq_iter->slot)
> > + hwsp_seq_iter->slot = radix_tree_next_chunk(&hwspinlock_tree,
> > &hwsp_seq_iter->iter, 0);
> > +
> > + return hwsp_seq_iter->slot ? hwsp_seq_iter : NULL;
>
> [Severity: High]
> Does returning NULL here cause a memory leak of the sequence iterator?
>
> When iteration reaches the end of the radix tree, hwspinlock_seq_next()
> returns NULL to signal EOF.
>
> The seq_file framework then updates its internal iterator pointer to NULL and
> calls hwspinlock_seq_stop() with v=NULL. Since stop() expects to free the
> iterator, calling kfree(NULL) means the originally allocated iterator is lost
> and never freed.
Uh, yes, overlooked that.
> > +}
> > +
> > +static void hwspinlock_seq_stop(struct seq_file *s, void *v)
> > +{
> > + kfree(v);
> > + mutex_unlock(&hwspinlock_tree_lock);
> > +}
> > +
> > +static int hwspinlock_seq_show(struct seq_file *s, void *v)
> > +{
> > + struct hwspinlock_seq_iterator *hwsp_seq_iter = v;
> > + unsigned long id = hwsp_seq_iter->iter.index;
> > + struct hwspinlock *hwlock;
> > + int used;
> > +
> > + used = radix_tree_tag_get(&hwspinlock_tree, id, HWSPINLOCK_USED);
> > + hwlock = radix_tree_deref_slot(hwsp_seq_iter->slot);
>
> [Severity: High]
> Does calling radix_tree_deref_slot() without an active RCU read lock trigger
> a lockdep warning?
True. For such cases, radix_tree_deref_slot_protected() exists. But it
only targets spinlocks not mutexes. Will think about it.
signature.asc
Description: PGP signature
