Re: [RFC PATCH net-next 2/2] net: dcb: add new apptrust attribute

Daniel.Machon
2022-09-13T19:22:25Z

Hi Petr,

> <Daniel.Machon@microchip.com> writes:
> >
> > Petr Machata <petrm@nvidia.com> writes:
> >>
> >> But of course this will never get anywhere close to that. We will end up
> >> passing maybe one, two entries. So the UAPI seems excessive in how it
> >> hands around this large array.
> >>
> >> I wonder if it would be better to make the DCB_ATTR_IEEE_APP_TABLE
> >> payload be an array of bytes, each byte a selector? Or something similar
> >> to DCB_ATTR_IEEE_APP_TABLE / DCB_ATTR_IEEE_APP, a nest and an array of
> >> payload attributes?
> >
> > Hmm. It might seem excessive, but a quick few thoughts on your proposed solution:
> > - We need more code to define and parse the new DCB_ATTR_IEEE_APP_TRUST_TABLE /
> > DCB_ATTR_IEEE_APP_TRUST attributes.
>
> Yes, a bit. But it's not too bad IMHO. Am I forgetting something here?
>
> u8 selectors[256];
> int nselectors;
> int rem;
>
> nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE], rem) {
> if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
> nla_len(attr) != 1 ||
> nselectors >= sizeof(selectors)) {
> err = -EINVAL;
> goto err;
> }
>
> selectors[nselectors++] = nla_get_u8(attr);
> }
>
> ... and you have reconstructed the array.

LGTM.

>
> > - If the selectors are passed individually to the driver, we need a
> > dcbnl_delapptrust(), because now, the driver have to add and del from the
> > driver maintained array. You could of course accumulate selectors in an array
> > before passing them to the driver, but then why go away from the array in the
> > first place.
>
> I have no problem with using an array for the in-kernel API. There it's
> easy to change. UAPI can't ever change.
>
> >> > + struct ieee_apptrust *trust =
> >> > + nla_data(ieee[DCB_ATTR_IEEE_APP_TRUST]);
> >>
> >> Besides invoking the OP, this should validate the payload. E.g. no
> >> driver is supposed to accept trust policies that contain invalid
> >> selectors. Pretty sure there's no value in repeated entries either.
> >
> > Validation (bogus input and unique selectors) is done in userspace (dcb-apptrust).
>
> Using iproute2 dcb is not mandatory, the UAPI is client-agnostic. The
> kernel needs to bounce bogons as well. Otherwise they will become part
> of the UAPI with the meaning "this doesn't do anything".
>
> And yeah, drivers will validate supported configurations. But still the
> requests that go to the driver should already be sanitized, so that the
> driver code doesn't need to worry about this.

Good point.

Will prepare a v2 with suggested changes.