On Wed, 12 Mar 2025 08:04:39 -0700 Joshua Hahn <joshua.hahnjy@xxxxxxxxx> wrote:
Hi Joshua
Thank you for your response regarding this patch.
> Hi Rakie, thank your new revision!
>
> I think this new ordering of the series makes more sense, since the bug exists
> even before your patch is applied! IMHO, it might also make sense to take
> patch 1 out of this series, and send it separately (and make patches 2-4
> their own series).
>
> I have a nit and a few thoughts about this patch and the way we order locking
> and allocating:
>
> > static void sysfs_wi_release(struct kobject *wi_kobj)
> > @@ -3464,35 +3470,54 @@ static const struct kobj_type wi_ktype = {
> >
> > static int sysfs_wi_node_add(int nid)
> > {
> > - struct iw_node_attr *node_attr;
> > + int ret = 0;
> > char *name;
> >
> > - node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> > - if (!node_attr)
> > - return -ENOMEM;
> > + if (nid < 0 || nid >= nr_node_ids) {
> > + pr_err("Invalid node id: %d\n", nid);
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + mutex_lock(&ngrp->kobj_lock);
> > + if (!ngrp->nattrs[nid]) {
> > + ngrp->nattrs[nid] = kzalloc(sizeof(struct iw_node_attr), GFP_KERNEL);
>
> I am unsure if kzallocing with the mutex_lock held is best practice. Even though
> two threads won't reach this place simultaneously since *most* calls to this
> function are sequential, we should try to keep the code safe since future
> patches might overlook this, and later make non-sequential calls : -)
>
> It also doesn't seem wise to directly assign the result of an allocation
> without checking for its success (as I explain below).
>
> IMHO it is best to allocate first, then acquire the lock and check for
> existence, then assign with the lock still held. We can also reduce this code
> section into a single if statement for clarity (but if you think it looks
> cleaner with the if-else, please keep it!)
>
> struct iw_node_attr *node_attr;
>
> ...
>
> node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
> if (!node_attr) {
> ret = -ENOMEM;
> goto out;
> }
>
> mutex_lock(&ngrp->kobj_lock);
> if (ngrp->nattrs[nid]) {
> mutex_unlock(&ngrp->kobj_lock);
> kfree(node_attr);
> pr_info("Node [%d] already exists\n");
> goto out;
> }
> ngrp->attrs[nid] = node_attr;
> mutex_unlock(&ngrp->kobj_lock):
>
Your suggestion makes sense, and I will update this part accordingly
to reflect your feedback.
>
> > + } else {
> > + mutex_unlock(&ngrp->kobj_lock);
> > + pr_info("Node [%d] is already existed\n", nid);
>
> NIT: To keep consistency with other parts of the kernel, maybe this can be
> rephrased to "Node [%d] already exists\n"
I also agree that modifying the wording would improve clarity.
I will make the necessary adjustments in the next version.
>
> > + goto out;
> > + }
> > + mutex_unlock(&ngrp->kobj_lock);
> > +
> > + if (!ngrp->nattrs[nid]) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
>
> If we make the changes above, we don't have to check for allocation success
> *after* already having locked & unlocked and making the unnecessary assignment.
>
> >
> > name = kasprintf(GFP_KERNEL, "node%d", nid);
> > if (!name) {
> > - kfree(node_attr);
> > - return -ENOMEM;
> > + kfree(ngrp->nattrs[nid]);
> > + ret = -ENOMEM;
> > + goto out;
> > }
>
> For the same reasons above, I think it makes sense to make this allocation
> together with the allocation of node_attr above, and free / return -ENOMEM
> as early as possible if we can.
>
> [...snip...]
>
> Thank you again for this patch! Please let me know what you think : -)
> Have a great day!
> Joshua
Thank you for your detailed and thoughtful review. I will incorporate
your suggestions and update the next version accordingly.
>
> Sent using hkml (https://github.com/sjp38/hackermail)
>
