Re: [PATCH] s390: correct misuses of module_put in appldata_generic_handler.

[Zhouyi Zhou]

Delicate design!  thanks for replying, sorry for the trouble

On Tue, Mar 18, 2014 at 9:34 PM, Gerald Schaefer
<gerald.schaefer@xxxxxxxxxx> wrote:
> On Tue, 18 Mar 2014 09:24:55 +0800
> Zhouyi Zhou <zhouzhouyi@xxxxxxxxx> wrote:
>
>> Thanks Gerald for reviewing and sorry for not elaborated it in the e-mail.
>>
>> Firstly, I think you can't call module_put after fail try_module_get
>
> There is another try_module_get() in this function before that, so I need
> to call module_put() once if the second try_module_get() fails.
>
> The first try_module_get() is only needed within the function, while the
> second one is needed to prevent a module from being unloaded while its
> callback is registered, so that reference is kept as long as the module
> is active.
>
>>
>> Secondly, there exists duplicate module_put on the program path (the last
>> one is before return 0)
>
> There is one module_put() for every try_module_get(). If "1" is written
> to the sysctl, the function exits with one "missing" module_put(), which
> is held to protect the callback. When "0" is written, this reference
> is returned again with an "extra" module_put().
>
>>
>> On Mon, Mar 17, 2014 at 9:28 PM, Gerald Schaefer
>> <gerald.schaefer@xxxxxxxxxx> wrote:
>> > On Sat, 15 Mar 2014 21:35:40 +0800
>> > Zhouyi Zhou <zhouzhouyi@xxxxxxxxx> wrote:
>> >
>> >> correct misuses of module_put in  appldata_generic_handler
>> >
>> > Sorry, I don't see any misuse, could you elaborate?
>> >
>> >>
>> >> Signed-off-by: Zhouyi Zhou <yizhouzhou@xxxxxxxxx>
>> >> ---
>> >>  arch/s390/appldata/appldata_base.c | 3 ---
>> >>  1 file changed, 3 deletions(-)
>> >>
>> >> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
>> >> index 47c8630..683e0282 100644
>> >> --- a/arch/s390/appldata/appldata_base.c
>> >> +++ b/arch/s390/appldata/appldata_base.c
>> >> @@ -343,7 +343,6 @@ appldata_generic_handler(struct ctl_table *ctl, int write,
>> >>               // protect work queue callback
>> >>               if (!try_module_get(ops->owner)) {
>> >>                       mutex_unlock(&appldata_ops_mutex);
>> >> -                     module_put(ops->owner);
>> >>                       return -ENODEV;
>> >>               }
>> >>               ops->callback(ops->data);       // init record
>> >> @@ -354,7 +353,6 @@ appldata_generic_handler(struct ctl_table *ctl, int write,
>> >>               if (rc != 0) {
>> >>                       pr_err("Starting the data collection for %s "
>> >>                              "failed with rc=%d\n", ops->name, rc);
>> >> -                     module_put(ops->owner);
>> >>               } else
>> >>                       ops->active = 1;
>> >>       } else if ((buf[0] == '0') && (ops->active == 1)) {
>> >> @@ -365,7 +363,6 @@ appldata_generic_handler(struct ctl_table *ctl, int write,
>> >>               if (rc != 0)
>> >>                       pr_err("Stopping the data collection for %s "
>> >>                              "failed with rc=%d\n", ops->name, rc);
>> >> -             module_put(ops->owner);
>> >>       }
>> >>       mutex_unlock(&appldata_ops_mutex);
>> >>  out:
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-s390" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/