On Wed, 2019-06-19 at 09:07 +0300, Leon Romanovsky wrote:
> On Tue, Jun 18, 2019 at 11:19:58PM -0400, Doug Ledford wrote:
> > On Tue, 2019-06-18 at 22:45 -0400, Doug Ledford wrote:
> > > On Tue, 2019-06-18 at 16:58 -0400, Doug Ledford wrote:
> > > > On Tue, 2019-06-18 at 15:46 -0300, Jason Gunthorpe wrote:
> > > > > On Tue, Jun 18, 2019 at 07:53:38PM +0300, Leon Romanovsky
> > > > > wrote:
> > > > > > I have a very strong opinion about it.
> > > > >
> > > > > Then Doug should add the policies, here are the output values
> > > > > from
> > > > > the
> > > > > userspace:
> > > > >
> > > > > [RDMA_NLDEV_ATTR_CHARDEV] = { .type = NLA_U64 },
> > > > > [RDMA_NLDEV_ATTR_CHARDEV_ABI] = { .type = NLA_U64 },
> > > > > [RDMA_NLDEV_ATTR_DEV_INDEX] = { .type = NLA_U32 },
> > > > > [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 },
> > > > > [RDMA_NLDEV_ATTR_NODE_GUID] = { .type = NLA_U64 },
> > > > > [RDMA_NLDEV_ATTR_UVERBS_DRIVER_ID] = { .type = NLA_U32
> > > > > },
> > > > > [RDMA_NLDEV_ATTR_CHARDEV_NAME] = { .type =
> > > > > NLA_NUL_STRING
> > > > > },
> > > > > [RDMA_NLDEV_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING
> > > > > },
> > > > > [RDMA_NLDEV_ATTR_DEV_PROTOCOL] = { .type =
> > > > > NLA_NUL_STRING
> > > > > },
> > > > > [RDMA_NLDEV_ATTR_FW_VERSION] = { .type =
> > > > > NLA_NUL_STRING },
> > > >
> > > > Most of those were already in the policies. Only the four that
> > > > you
> > > > added to enum rdma_nldev_attr needed added to the policies, and
> > > > two
> > > > of
> > > > them your patch already added. The only question I had is what
> > > > the
> > > > string length should be on ATTR_CHARDEV_NAME? I throw in the
> > > > default
> > > > of
> > > > .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN, but I wasn't sure if that
> > > > was
> > > > right
> > > > for this entry?
> > >
> > > First of all, let me say that this is a PITA. I'm thinking we
> > > need to
> > > order all of the attributes in the policy array in alphabetical
> > > order
> > > so
> > > it's easier to tell what's there and what's missing.
> > >
> > > Also, I'm starting to not like adding items to the policy array
> > > before
> > > they are actually acceptable as inputs. Sure, there's the
> > > argument
> > > that
> > > they won't get missed. But there's the counter argument that
> > > until
> > > you
> > > define them as an input in some netlink function that reads them,
> > > at
> > > least for all of the string items, you can't actually set the real
> > > string length limit. By adding them to the policy now, you make
> > > it a
> > > guarantee that you'll end up changing the API under userspace
> > > later
> > > when
> > > they become a legitimate input. Not sure if that's actually
> > > wise. The
> > > u32 and u64, sure, that's fine. But the strings, that can cause
> > > problems later.,
> > >
> >
> > Maybe the best way to do string items if you add them ahead of time
> > is
> > to give them an input length of 1. Theoretically that's just the
> > null
> > byte. Then when you add a function to actually read the data, set
> > the
> > size to something real.
>
> Like you said, it won't be needed till new input code is used, so from
> API perspective, it doesn't matter what size will be. I liked your
> idea
> to put it to be "1". It goes hand by hand to avoid possible addition
> of input handlers without update of update nla_policy.
Ok, I cleaned up the policy array by sorting it so you can find things
when you need to add new stuff. New policy additions should preserve
the sorting. Then I audited all of the string inputs, fixed their
definitions in the policy, and removed the (now) unnecessary checks for
string input overflow. Their already in my wip branch, but I'll send
them to the list too.
> > Anyway, pushed to wip/dl-for-next, check it to make sure you're
> > happy
> > with the final results.
>
> It looks good, thanks.
>
> > --
> > Doug Ledford <dledford@xxxxxxxxxx>
> > GPG KeyID: B826A3330E572FDD
> > Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
>
>
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
