lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221207083014.mipiiu6dzgpuayz5@soft-dev3-1>
Date:   Wed, 7 Dec 2022 09:30:14 +0100
From:   Horatiu Vultur <horatiu.vultur@...rochip.com>
To:     Paolo Abeni <pabeni@...hat.com>
CC:     <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>,
        <Steen.Hegelund@...rochip.com>, <lars.povlsen@...rochip.com>,
        <daniel.machon@...rochip.com>, <richardcochran@...il.com>,
        <UNGLinuxDriver@...rochip.com>, <olteanv@...il.com>
Subject: Re: [PATCH net-next v3 1/4] net: microchip: vcap: Add vcap_get_rule

The 12/06/2022 13:31, Paolo Abeni wrote:
> 
> Hello,

Hi Paolo,

> 
> On Sat, 2022-12-03 at 11:43 +0100, Horatiu Vultur wrote:
> > @@ -632,32 +264,22 @@ static int vcap_show_admin(struct vcap_control *vctrl,
> >                          struct vcap_admin *admin,
> >                          struct vcap_output_print *out)
> >  {
> > -     struct vcap_rule_internal *elem, *ri;
> > +     struct vcap_rule_internal *elem;
> > +     struct vcap_rule *vrule;
> >       int ret = 0;
> >
> >       vcap_show_admin_info(vctrl, admin, out);
> > -     mutex_lock(&admin->lock);
> >       list_for_each_entry(elem, &admin->rules, list) {
> 
> Not strictly related to this patch, as the patter is AFAICS already
> there in other places, but I'd like to understand the admin->rules
> locking schema.

According to the commit message that introduced this lock [0] and the
comment to the lock, this lock is used to protect the access to the
admin->rules, admin->enabled and the caches which means the access to
the HW (to read/write the rules).
> 
> It looks like addition/removal are protected by admin->lock, but
> traversal is usually lockless, which in turn looks buggy ?!?

Thanks for the observation! You are correct, there seems to be some bugs
regarding the usage of this lock. We will look over this and will send a
patch to fix this.

> 
> Note: as this patch does not introduce the mentioned behavior, I'm not
> going to block the series for the above.

[0] 71c9de995260 ("net: microchip: sparx5: Add VCAP locking to protect rules")
> 
> Thanks,
> 
> Paolo
> > -             ri = vcap_dup_rule(elem);
> > -             if (IS_ERR(ri)) {
> > -                     ret = PTR_ERR(ri);
> > -                     goto err_unlock;
> > +             vrule = vcap_get_rule(vctrl, elem->data.id);
> > +             if (IS_ERR_OR_NULL(vrule)) {
> > +                     ret = PTR_ERR(vrule);
> > +                     break;
> >               }
> > -             /* Read data from VCAP */
> > -             ret = vcap_read_rule(ri);
> > -             if (ret)
> > -                     goto err_free_rule;
> > +
> >               out->prf(out->dst, "\n");
> > -             vcap_show_admin_rule(vctrl, admin, out, ri);
> > -             vcap_free_rule((struct vcap_rule *)ri);
> > +             vcap_show_admin_rule(vctrl, admin, out, to_intrule(vrule));
> > +             vcap_free_rule(vrule);
> >       }
> > -     mutex_unlock(&admin->lock);
> > -     return 0;
> > -
> > -err_free_rule:
> > -     vcap_free_rule((struct vcap_rule *)ri);
> > -err_unlock:
> > -     mutex_unlock(&admin->lock);
> >       return ret;
> >  }
> 

-- 
/Horatiu

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ