linux-kernel - Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table

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: <Y8f4i2ablWnNO9Op@DEN-LT-70577>
Date:   Wed, 18 Jan 2023 13:47:57 +0000
From:   <Daniel.Machon@...rochip.com>
To:     <petrm@...dia.com>
CC:     <netdev@...r.kernel.org>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <Lars.Povlsen@...rochip.com>, <Steen.Hegelund@...rochip.com>,
        <UNGLinuxDriver@...rochip.com>, <joe@...ches.com>,
        <error27@...il.com>, <Horatiu.Vultur@...rochip.com>,
        <Julia.Lawall@...ia.fr>, <vladimir.oltean@....com>,
        <maxime.chevallier@...tlin.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 3/6] net: dcb: add new rewrite table

> > +     rewr = nla_nest_start_noflag(skb, DCB_ATTR_DCB_REWR_TABLE);
> > +     if (!rewr)
> > +             return -EMSGSIZE;
> 
> This being new code, don't use _noflag please.

Ack.

> 
> > +
> > +     spin_lock_bh(&dcb_lock);
> > +     list_for_each_entry(itr, &dcb_rewr_list, list) {
> > +             if (itr->ifindex == netdev->ifindex) {
> > +                     enum ieee_attrs_app type =
> > +                             dcbnl_app_attr_type_get(itr->app.selector);
> > +                     err = nla_put(skb, type, sizeof(itr->app), &itr->app);
> > +                     if (err) {
> > +                             spin_unlock_bh(&dcb_lock);
> 
> This should cancel the nest started above.

Yes, it should.

> 
> I wonder if it would be cleaner in a separate function, so that there
> can be a dedicated clean-up block to goto.

Well yes. That would make sense if the function were reused for both APP
and rewr.

Though in the APP equivalent code, nla_nest_start_noflag is used, and
dcbnl_ops->getdcbx() is called. Is there any userspace side-effect of
using nla_nest_start for APP too?

dcbnl_ops->getdcbx() would then be left outside of the shared function.
Does that call even have to hold the dcb_lock? Not as far as I can tell.

something like:

err = dcbnl_app_table_get(ndev, skb, &dcb_app_list,
			  DCB_ATTR_IEEE_APP_TABLE);
if (err)
	return -EMSGSIZE;

err = dcbnl_app_table_get(ndev, skb, &dcb_rewr_list,
			  DCB_ATTR_DCB_REWR_TABLE);
if (err)
        return -EMSGSIZE;

if (netdev->dcbnl_ops->getdcbx)
	dcbx = netdev->dcbnl_ops->getdcbx(netdev); <-- without lock held
else
	dcbx = -EOPNOTSUPP;

Let me hear your thoughts.