Re: [PATCH v6 1/3] devres: provide devm_krealloc()

[Bartosz Golaszewski]

On Sun, Aug 2, 2020 at 12:42 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>

[snip]

>
> I was thinking about this bit... Shouldn't we rather issue a simple
> dev_warn() and return the existing pointer?
> For example in some cases we might want to have resources coming
> either from heap or from constant. Then, if at some circumstances we
> would like to extend that memory (only for non-constant cases) we
> would need to manage this ourselves. Otherwise we may simply call
> krealloc().
> It seems that devm_kstrdup_const returns an initial pointer. Getting
> NULL is kinda inconvenient (and actually dev_warn() might also be
> quite a noise, however I would give a message to the user, because
> it's something worth checking).
>

But this is inconsistent behavior: if you pass a pointer to ro memory
to devm_krealloc() it will not resize it but by returning a valid
pointer it will make you think it did -> you end up writing to ro
memory in good faith.

> ...
>
> > +       spin_lock_irqsave(&dev->devres_lock, flags);
> > +       old_dr = find_dr(dev, devm_kmalloc_release, devm_kmalloc_match, ptr);
> > +       spin_unlock_irqrestore(&dev->devres_lock, flags);
>
> > +       if (!old_dr) {
>
> I would have this under spin lock b/c of below.
>
> > +               WARN(1, "Memory chunk not managed or managed by a different device.");
> > +               return NULL;
> > +       }
>
> > +       old_head = old_dr->node.entry;
>
> This would be still better to be under spin lock.
>
> > +       new_dr = krealloc(old_dr, total_size, gfp);
> > +       if (!new_dr)
> > +               return NULL;
>
> And perhaps spin lock taken already here.
>
> > +       if (new_dr != old_dr) {
> > +               spin_lock_irqsave(&dev->devres_lock, flags);
> > +               list_replace(&old_head, &new_dr->node.entry);
> > +               spin_unlock_irqrestore(&dev->devres_lock, flags);
> > +       }
>
> Yes, I understand that covering more code under spin lock does not fix
> any potential race, but at least it minimizes scope of the code that
> is not under it to see exactly what is problematic.
>
> I probably will think more about a better approach to avoid potential races.

My thinking behind this was this: we already have users who call
devres_find() and do something with the retrieved resources without
holding the devres_lock - so it's assumed that users are sane enough
to not be getting in each other's way. Now I see that the difference
is that here we're accessing the list node and it can change if
another thread is adding a different devres to the same device. So
this should definitely be protected somehow.

I think that we may have to give up using real krealloc() and instead
just reimplement its behavior in the following way:

Still outside of spinlock check if the new total size is smaller or
equal to the previous one. If so: return the same pointer. If not:
allocate a new devres as if it were for devm_kmalloc() but don't add
it to the list yet. Take the spinlock - check if we can find the
devres - if not: kfree() the new and old chunk and return NULL. If
yes: copy the contents of the devres node into the new chunk as well
as the memory contents. Replace the old one on the list and free it.
Release spinlock and return.

Does that work?

Bart