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

Daniel.Machon
2023-01-18T14:09:16Z

> > + 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.