Re: [PATCH 2/2] hwspinlock: add summary in debugfs

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3337 bytes --]

> > +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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]