On Mon, Sep 3, 2018 at 10:12 AM Vlad Buslov <vla...@mellanox.com> wrote:
>
>
> On Mon 03 Sep 2018 at 17:03, Cong Wang <xiyou.wangc...@gmail.com> wrote:
> > On Mon, Sep 3, 2018 at 12:10 AM Vlad Buslov <vla...@mellanox.com> wrote:
> >>
> >> Recent refactoring of add_metainfo() caused use_all_metadata() to add
> >> metainfo to ife action metalist without taking reference to module. This
> >> causes warning in module_put called from ife action cleanup function.
> >>
> >> Implement add_metainfo_and_get_ops() function that returns with reference
> >> to module taken if metainfo was added successfully, and call it from
> >> use_all_metadata(), instead of calling __add_metainfo() directly.
> >
> > Good catch!
> >
> > I thought every entry in ifeoplist must hold a refcnt to its module, looks
> > like I was wrong.
> >
> >
> >> read_lock(&ife_mod_lock);
> >> list_for_each_entry(o, &ifeoplist, list) {
> >> - rc = __add_metainfo(o, ife, o->metaid, NULL, 0, true,
> >> exists);
> >> + rc = add_metainfo_and_get_ops(o, ife, o->metaid, NULL, 0,
> >> true,
> >> + exists);
> >> if (rc == 0)
> >> installed += 1;
> >
> > I am afraid you have to rollback on failure inside this loop, that is,
> > releasing all previous module refcnt properly on error.
>
> Do I? This function looks like it is explicitly designed to succeed if
> at least one metainfo was successfully added. And this is how it was
> originally implemented before deadlock fix refactoring.
Hmm, well, it is taken care by tcf_idr_release() via _tcf_ife_cleanup()...
So, your patch should be fine.
