[PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Danielle Ratson]

NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte one
if necessary.

There are some NLA_UINT attributes that lack an appropriate getter function.

Add a function mnl_attr_get_uint() to cover that extract these. Since we
need to dispatch on length anyway, make the getter truly universal by
supporting also u8 and u16.

Signed-off-by: Danielle Ratson <danieller@nvidia.com>
---
 include/libmnl/libmnl.h |  1 +
 src/attr.c              | 22 ++++++++++++++++++++++
 src/libmnl.map          |  4 ++++
 3 files changed, 27 insertions(+)

diff --git a/include/libmnl/libmnl.h b/include/libmnl/libmnl.h
index 4bd0b92..9c03280 100644
--- a/include/libmnl/libmnl.h
+++ b/include/libmnl/libmnl.h
@@ -92,6 +92,7 @@ extern uint8_t mnl_attr_get_u8(const struct nlattr *attr);
 extern uint16_t mnl_attr_get_u16(const struct nlattr *attr);
 extern uint32_t mnl_attr_get_u32(const struct nlattr *attr);
 extern uint64_t mnl_attr_get_u64(const struct nlattr *attr);
+extern uint64_t mnl_attr_get_uint(const struct nlattr *attr);
 extern const char *mnl_attr_get_str(const struct nlattr *attr);
 
 /* TLV attribute putters */
diff --git a/src/attr.c b/src/attr.c
index bc39df4..399318e 100644
--- a/src/attr.c
+++ b/src/attr.c
@@ -389,6 +389,28 @@ EXPORT_SYMBOL uint64_t mnl_attr_get_u64(const struct nlattr *attr)
 	return tmp;
 }
 
+/**
+ * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
+ * \param attr pointer to netlink attribute
+ *
+ * This function returns the 64-bit value of the attribute payload.
+ */
+EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
+{
+	switch (mnl_attr_get_payload_len(attr)) {
+	case sizeof(uint8_t):
+		return mnl_attr_get_u8(attr);
+	case sizeof(uint16_t):
+		return mnl_attr_get_u16(attr);
+	case sizeof(uint32_t):
+		return mnl_attr_get_u32(attr);
+	case sizeof(uint64_t):
+		return mnl_attr_get_u64(attr);
+	}
+
+	return -1ULL;
+}
+
 /**
  * mnl_attr_get_str - get pointer to string attribute
  * \param attr pointer to netlink attribute
diff --git a/src/libmnl.map b/src/libmnl.map
index e5920e5..cd58863 100644
--- a/src/libmnl.map
+++ b/src/libmnl.map
@@ -77,3 +77,7 @@ LIBMNL_1.2 {
   mnl_socket_open2;
   mnl_socket_fdopen;
 } LIBMNL_1.1;
+
+LIBMNL_1.3 {
+  mnl_attr_get_uint;
+} LIBMNL_1.2;
-- 
2.45.0

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Phil Sutter]

On Wed, Jul 31, 2024 at 09:35:51AM +0300, Danielle Ratson wrote:
> NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte one
> if necessary.
> 
> There are some NLA_UINT attributes that lack an appropriate getter function.
> 
> Add a function mnl_attr_get_uint() to cover that extract these. Since we
> need to dispatch on length anyway, make the getter truly universal by
> supporting also u8 and u16.
> 
> Signed-off-by: Danielle Ratson <danieller@nvidia.com>

Patch applied, thanks!

RE: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Danielle Ratson]

Hi,

Is there a plan to build a new version soon? 
I am asking since I am planning to use this function in ethtool.

Thanks,
Danielle

> -----Original Message-----
> From: Phil Sutter <phil@nwl.cc>
> Sent: Wednesday, 31 July 2024 10:15
> To: Danielle Ratson <danieller@nvidia.com>
> Cc: netfilter-devel@vger.kernel.org; pablo@netfilter.org; fw@strlen.de; mlxsw
> <mlxsw@nvidia.com>
> Subject: Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function
> 
> On Wed, Jul 31, 2024 at 09:35:51AM +0300, Danielle Ratson wrote:
> > NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte
> > one if necessary.
> >
> > There are some NLA_UINT attributes that lack an appropriate getter
> function.
> >
> > Add a function mnl_attr_get_uint() to cover that extract these. Since
> > we need to dispatch on length anyway, make the getter truly universal
> > by supporting also u8 and u16.
> >
> > Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> 
> Patch applied, thanks!

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:
> Hi,
> 
> Is there a plan to build a new version soon? 
> I am asking since I am planning to use this function in ethtool.

ASAP

> Thanks,
> Danielle
> 
> > -----Original Message-----
> > From: Phil Sutter <phil@nwl.cc>
> > Sent: Wednesday, 31 July 2024 10:15
> > To: Danielle Ratson <danieller@nvidia.com>
> > Cc: netfilter-devel@vger.kernel.org; pablo@netfilter.org; fw@strlen.de; mlxsw
> > <mlxsw@nvidia.com>
> > Subject: Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function
> > 
> > On Wed, Jul 31, 2024 at 09:35:51AM +0300, Danielle Ratson wrote:
> > > NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte
> > > one if necessary.
> > >
> > > There are some NLA_UINT attributes that lack an appropriate getter
> > function.
> > >
> > > Add a function mnl_attr_get_uint() to cover that extract these. Since
> > > we need to dispatch on length anyway, make the getter truly universal
> > > by supporting also u8 and u16.
> > >
> > > Signed-off-by: Danielle Ratson <danieller@nvidia.com>
> > 
> > Patch applied, thanks!

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

Cc'ing Jakub.

On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:
> > Hi,
> > 
> > Is there a plan to build a new version soon? 
> > I am asking since I am planning to use this function in ethtool.
> 
> ASAP

but one question before... Is this related to NLA_UINT in the kernel?

/**
 * nla_put_uint - Add a variable-size unsigned int to a socket buffer
 * @skb: socket buffer to add attribute to
 * @attrtype: attribute type
 * @value: numeric value
 */
static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
{
        u64 tmp64 = value;
        u32 tmp32 = value;

        if (tmp64 == tmp32)
                return nla_put_u32(skb, attrtype, tmp32);
        return nla_put(skb, attrtype, sizeof(u64), &tmp64);
}

if I'm correct, it seems kernel always uses either u32 or u64.

Userspace assumes u8 and u16 are possible though:

+/**
+ * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
+ * \param attr pointer to netlink attribute
+ *
+ * This function returns the 64-bit value of the attribute payload.
+ */
+EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
+{
+       switch (mnl_attr_get_payload_len(attr)) {
+       case sizeof(uint8_t):
+               return mnl_attr_get_u8(attr);
+       case sizeof(uint16_t):
+               return mnl_attr_get_u16(attr);
+       case sizeof(uint32_t):
+               return mnl_attr_get_u32(attr);
+       case sizeof(uint64_t):
+               return mnl_attr_get_u64(attr);
+       }
+
+       return -1ULL;
+}

Or this is an attempt to provide a helper that allows you fetch for
payload value of 2^3..2^6 bytes?

Thanks.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Jakub Kicinski]

On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
> > > Hi,
> > > 
> > > Is there a plan to build a new version soon? 
> > > I am asking since I am planning to use this function in ethtool.  
> > 
> > ASAP  
> 
> but one question before... Is this related to NLA_UINT in the kernel?
> 
> /**
>  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
>  * @skb: socket buffer to add attribute to
>  * @attrtype: attribute type
>  * @value: numeric value
>  */
> static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
> {
>         u64 tmp64 = value;
>         u32 tmp32 = value;
> 
>         if (tmp64 == tmp32)
>                 return nla_put_u32(skb, attrtype, tmp32);
>         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
> }
> 
> if I'm correct, it seems kernel always uses either u32 or u64.
> 
> Userspace assumes u8 and u16 are possible though:
> 
> +/**
> + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> + * \param attr pointer to netlink attribute
> + *
> + * This function returns the 64-bit value of the attribute payload.
> + */
> +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> +{
> +       switch (mnl_attr_get_payload_len(attr)) {
> +       case sizeof(uint8_t):
> +               return mnl_attr_get_u8(attr);
> +       case sizeof(uint16_t):
> +               return mnl_attr_get_u16(attr);
> +       case sizeof(uint32_t):
> +               return mnl_attr_get_u32(attr);
> +       case sizeof(uint64_t):
> +               return mnl_attr_get_u64(attr);
> +       }
> +
> +       return -1ULL;
> +}
> 
> Or this is an attempt to provide a helper that allows you fetch for
> payload value of 2^3..2^6 bytes?

No preference here, FWIW. Looks like this patch does a different thing
than the kernel. But maybe a broader "automatic" helper is useful for
user space code.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
> > > > Hi,
> > > > 
> > > > Is there a plan to build a new version soon? 
> > > > I am asking since I am planning to use this function in ethtool.  
> > > 
> > > ASAP  
> > 
> > but one question before... Is this related to NLA_UINT in the kernel?
> > 
> > /**
> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
> >  * @skb: socket buffer to add attribute to
> >  * @attrtype: attribute type
> >  * @value: numeric value
> >  */
> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
> > {
> >         u64 tmp64 = value;
> >         u32 tmp32 = value;
> > 
> >         if (tmp64 == tmp32)
> >                 return nla_put_u32(skb, attrtype, tmp32);
> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
> > }
> > 
> > if I'm correct, it seems kernel always uses either u32 or u64.
> > 
> > Userspace assumes u8 and u16 are possible though:
> > 
> > +/**
> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> > + * \param attr pointer to netlink attribute
> > + *
> > + * This function returns the 64-bit value of the attribute payload.
> > + */
> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> > +{
> > +       switch (mnl_attr_get_payload_len(attr)) {
> > +       case sizeof(uint8_t):
> > +               return mnl_attr_get_u8(attr);
> > +       case sizeof(uint16_t):
> > +               return mnl_attr_get_u16(attr);
> > +       case sizeof(uint32_t):
> > +               return mnl_attr_get_u32(attr);
> > +       case sizeof(uint64_t):
> > +               return mnl_attr_get_u64(attr);
> > +       }
> > +
> > +       return -1ULL;
> > +}
> > 
> > Or this is an attempt to provide a helper that allows you fetch for
> > payload value of 2^3..2^6 bytes?
> 
> No preference here, FWIW. Looks like this patch does a different thing
> than the kernel. But maybe a broader "automatic" helper is useful for
> user space code.

Not sure. @Danielle: could you clarify your intention?

If this is to support NLA_UINT, I'd prefer to stick to NLA_UINT semantics.

@Jakub: is there any plan to augment NLA_UINT in the future? What
the assumption from userspace that this will always return 32-bits
else 64-bits value?

Thanks.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Petr Machata]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
>> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
>> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
>> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
>> > > > Hi,
>> > > > 
>> > > > Is there a plan to build a new version soon? 
>> > > > I am asking since I am planning to use this function in ethtool.  
>> > > 
>> > > ASAP  
>> > 
>> > but one question before... Is this related to NLA_UINT in the kernel?
>> > 
>> > /**
>> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
>> >  * @skb: socket buffer to add attribute to
>> >  * @attrtype: attribute type
>> >  * @value: numeric value
>> >  */
>> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
>> > {
>> >         u64 tmp64 = value;
>> >         u32 tmp32 = value;
>> > 
>> >         if (tmp64 == tmp32)
>> >                 return nla_put_u32(skb, attrtype, tmp32);
>> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
>> > }
>> > 
>> > if I'm correct, it seems kernel always uses either u32 or u64.
>> > 
>> > Userspace assumes u8 and u16 are possible though:
>> > 
>> > +/**
>> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
>> > + * \param attr pointer to netlink attribute
>> > + *
>> > + * This function returns the 64-bit value of the attribute payload.
>> > + */
>> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
>> > +{
>> > +       switch (mnl_attr_get_payload_len(attr)) {
>> > +       case sizeof(uint8_t):
>> > +               return mnl_attr_get_u8(attr);
>> > +       case sizeof(uint16_t):
>> > +               return mnl_attr_get_u16(attr);
>> > +       case sizeof(uint32_t):
>> > +               return mnl_attr_get_u32(attr);
>> > +       case sizeof(uint64_t):
>> > +               return mnl_attr_get_u64(attr);
>> > +       }
>> > +
>> > +       return -1ULL;
>> > +}
>> > 
>> > Or this is an attempt to provide a helper that allows you fetch for
>> > payload value of 2^3..2^6 bytes?
>> 
>> No preference here, FWIW. Looks like this patch does a different thing
>> than the kernel. But maybe a broader "automatic" helper is useful for
>> user space code.
>
> Not sure. @Danielle: could you clarify your intention?

This follows the iproute2 helper, where I was asked to support >32-bit
fields purely as a service to the users, so that one helper can be used
for any integral field.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
> 
> Pablo Neira Ayuso <pablo@netfilter.org> writes:
> 
> > On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> >> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> >> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> >> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
> >> > > > Hi,
> >> > > > 
> >> > > > Is there a plan to build a new version soon? 
> >> > > > I am asking since I am planning to use this function in ethtool.  
> >> > > 
> >> > > ASAP  
> >> > 
> >> > but one question before... Is this related to NLA_UINT in the kernel?
> >> > 
> >> > /**
> >> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
> >> >  * @skb: socket buffer to add attribute to
> >> >  * @attrtype: attribute type
> >> >  * @value: numeric value
> >> >  */
> >> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
> >> > {
> >> >         u64 tmp64 = value;
> >> >         u32 tmp32 = value;
> >> > 
> >> >         if (tmp64 == tmp32)
> >> >                 return nla_put_u32(skb, attrtype, tmp32);
> >> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
> >> > }
> >> > 
> >> > if I'm correct, it seems kernel always uses either u32 or u64.
> >> > 
> >> > Userspace assumes u8 and u16 are possible though:
> >> > 
> >> > +/**
> >> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> >> > + * \param attr pointer to netlink attribute
> >> > + *
> >> > + * This function returns the 64-bit value of the attribute payload.
> >> > + */
> >> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> >> > +{
> >> > +       switch (mnl_attr_get_payload_len(attr)) {
> >> > +       case sizeof(uint8_t):
> >> > +               return mnl_attr_get_u8(attr);
> >> > +       case sizeof(uint16_t):
> >> > +               return mnl_attr_get_u16(attr);
> >> > +       case sizeof(uint32_t):
> >> > +               return mnl_attr_get_u32(attr);
> >> > +       case sizeof(uint64_t):
> >> > +               return mnl_attr_get_u64(attr);
> >> > +       }
> >> > +
> >> > +       return -1ULL;
> >> > +}
> >> > 
> >> > Or this is an attempt to provide a helper that allows you fetch for
> >> > payload value of 2^3..2^6 bytes?
> >> 
> >> No preference here, FWIW. Looks like this patch does a different thing
> >> than the kernel. But maybe a broader "automatic" helper is useful for
> >> user space code.
> >
> > Not sure. @Danielle: could you clarify your intention?
> 
> This follows the iproute2 helper, where I was asked to support >32-bit
> fields purely as a service to the users, so that one helper can be used
> for any integral field.

Which helper are your referring to? Is it modeled after NLA_UINT?

I don't think this patch is fine. This also returns -1ULL so there is
no way to know if size is not correct or payload length is 64 bits
using UINT64_MAX?

Thanks.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

On Mon, Sep 30, 2024 at 07:01:42PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
> > 
> > Pablo Neira Ayuso <pablo@netfilter.org> writes:
> > 
> > > On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> > >> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> > >> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > >> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
> > >> > > > Hi,
> > >> > > > 
> > >> > > > Is there a plan to build a new version soon? 
> > >> > > > I am asking since I am planning to use this function in ethtool.  
> > >> > > 
> > >> > > ASAP  
> > >> > 
> > >> > but one question before... Is this related to NLA_UINT in the kernel?
> > >> > 
> > >> > /**
> > >> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
> > >> >  * @skb: socket buffer to add attribute to
> > >> >  * @attrtype: attribute type
> > >> >  * @value: numeric value
> > >> >  */
> > >> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
> > >> > {
> > >> >         u64 tmp64 = value;
> > >> >         u32 tmp32 = value;
> > >> > 
> > >> >         if (tmp64 == tmp32)
> > >> >                 return nla_put_u32(skb, attrtype, tmp32);
> > >> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
> > >> > }
> > >> > 
> > >> > if I'm correct, it seems kernel always uses either u32 or u64.
> > >> > 
> > >> > Userspace assumes u8 and u16 are possible though:
> > >> > 
> > >> > +/**
> > >> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> > >> > + * \param attr pointer to netlink attribute
> > >> > + *
> > >> > + * This function returns the 64-bit value of the attribute payload.
> > >> > + */
> > >> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> > >> > +{
> > >> > +       switch (mnl_attr_get_payload_len(attr)) {
> > >> > +       case sizeof(uint8_t):
> > >> > +               return mnl_attr_get_u8(attr);
> > >> > +       case sizeof(uint16_t):
> > >> > +               return mnl_attr_get_u16(attr);
> > >> > +       case sizeof(uint32_t):
> > >> > +               return mnl_attr_get_u32(attr);
> > >> > +       case sizeof(uint64_t):
> > >> > +               return mnl_attr_get_u64(attr);
> > >> > +       }
> > >> > +
> > >> > +       return -1ULL;
> > >> > +}
> > >> > 
> > >> > Or this is an attempt to provide a helper that allows you fetch for
> > >> > payload value of 2^3..2^6 bytes?
> > >> 
> > >> No preference here, FWIW. Looks like this patch does a different thing
> > >> than the kernel. But maybe a broader "automatic" helper is useful for
> > >> user space code.
> > >
> > > Not sure. @Danielle: could you clarify your intention?
> > 
> > This follows the iproute2 helper, where I was asked to support >32-bit
> > fields purely as a service to the users, so that one helper can be used
> > for any integral field.
> 
> Which helper are your referring to? Is it modeled after NLA_UINT?
> 
> I don't think this patch is fine. This also returns -1ULL so there is
> no way to know if size is not correct or payload length is 64 bits
> using UINT64_MAX?

I found it:

static inline __u64 rta_getattr_uint(const struct rtattr *rta)

This only has one user in the tree so far, right?

include/libnetlink.h:static inline __u64 rta_getattr_uint(const struct rtattr *rta)
ip/ipnexthop.c:                 nh_grp_stats->packets = rta_getattr_uint(rta);
ip/ipnexthop.c:                 nh_grp_stats->packets_hw = rta_getattr_uint(rta);

is this attribute for ipnexthop of NLA_UINT type?

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Pablo Neira Ayuso]

On Mon, Sep 30, 2024 at 07:10:27PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 30, 2024 at 07:01:42PM +0200, Pablo Neira Ayuso wrote:
> > On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
> > > 
> > > Pablo Neira Ayuso <pablo@netfilter.org> writes:
> > > 
> > > > On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> > > >> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> > > >> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > > >> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
> > > >> > > > Hi,
> > > >> > > > 
> > > >> > > > Is there a plan to build a new version soon? 
> > > >> > > > I am asking since I am planning to use this function in ethtool.  
> > > >> > > 
> > > >> > > ASAP  
> > > >> > 
> > > >> > but one question before... Is this related to NLA_UINT in the kernel?
> > > >> > 
> > > >> > /**
> > > >> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
> > > >> >  * @skb: socket buffer to add attribute to
> > > >> >  * @attrtype: attribute type
> > > >> >  * @value: numeric value
> > > >> >  */
> > > >> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
> > > >> > {
> > > >> >         u64 tmp64 = value;
> > > >> >         u32 tmp32 = value;
> > > >> > 
> > > >> >         if (tmp64 == tmp32)
> > > >> >                 return nla_put_u32(skb, attrtype, tmp32);
> > > >> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
> > > >> > }
> > > >> > 
> > > >> > if I'm correct, it seems kernel always uses either u32 or u64.
> > > >> > 
> > > >> > Userspace assumes u8 and u16 are possible though:
> > > >> > 
> > > >> > +/**
> > > >> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> > > >> > + * \param attr pointer to netlink attribute
> > > >> > + *
> > > >> > + * This function returns the 64-bit value of the attribute payload.
> > > >> > + */
> > > >> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> > > >> > +{
> > > >> > +       switch (mnl_attr_get_payload_len(attr)) {
> > > >> > +       case sizeof(uint8_t):
> > > >> > +               return mnl_attr_get_u8(attr);
> > > >> > +       case sizeof(uint16_t):
> > > >> > +               return mnl_attr_get_u16(attr);
> > > >> > +       case sizeof(uint32_t):
> > > >> > +               return mnl_attr_get_u32(attr);
> > > >> > +       case sizeof(uint64_t):
> > > >> > +               return mnl_attr_get_u64(attr);
> > > >> > +       }
> > > >> > +
> > > >> > +       return -1ULL;
> > > >> > +}
> > > >> > 
> > > >> > Or this is an attempt to provide a helper that allows you fetch for
> > > >> > payload value of 2^3..2^6 bytes?
> > > >> 
> > > >> No preference here, FWIW. Looks like this patch does a different thing
> > > >> than the kernel. But maybe a broader "automatic" helper is useful for
> > > >> user space code.
> > > >
> > > > Not sure. @Danielle: could you clarify your intention?
> > > 
> > > This follows the iproute2 helper, where I was asked to support >32-bit
> > > fields purely as a service to the users, so that one helper can be used
> > > for any integral field.
> > 
> > Which helper are your referring to? Is it modeled after NLA_UINT?
> > 
> > I don't think this patch is fine. This also returns -1ULL so there is
> > no way to know if size is not correct or payload length is 64 bits
> > using UINT64_MAX?
> 
> I found it:
> 
> static inline __u64 rta_getattr_uint(const struct rtattr *rta)
> 
> This only has one user in the tree so far, right?

Well, this is a matter of documenting behaviour.

> include/libnetlink.h:static inline __u64 rta_getattr_uint(const struct rtattr *rta)
> ip/ipnexthop.c:                 nh_grp_stats->packets = rta_getattr_uint(rta);
> ip/ipnexthop.c:                 nh_grp_stats->packets_hw = rta_getattr_uint(rta);
> 
> is this attribute for ipnexthop of NLA_UINT type?

But it seems intention is to support NLA_UINT according to iproute's
commit.

commit 95836fbf35d352f7c031ddac2e6093a935308cc9
Author: Petr Machata <petrm@nvidia.com>
Date:   Thu Mar 14 15:52:12 2024 +0100

    libnetlink: Add rta_getattr_uint()
    
    NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte one if
    necessary. Add a function to extract these. Since we need to dispatch on
    length anyway, make the getter truly universal by supporting also u8 and
    u16.

so it went further to make it universal for 2^3..2^6 values.

I am going to submit a patch to provide more info on this helper function.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Petr Machata]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Mon, Sep 30, 2024 at 07:10:27PM +0200, Pablo Neira Ayuso wrote:
>> On Mon, Sep 30, 2024 at 07:01:42PM +0200, Pablo Neira Ayuso wrote:
>> > On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
>> > > 
>> > > Pablo Neira Ayuso <pablo@netfilter.org> writes:
>> > > 
>> > > > On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
>> > > >> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
>> > > >> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
>> > > >> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
>> > > >> > > > Hi,
>> > > >> > > > 
>> > > >> > > > Is there a plan to build a new version soon? 
>> > > >> > > > I am asking since I am planning to use this function in ethtool.  
>> > > >> > > 
>> > > >> > > ASAP  
>> > > >> > 
>> > > >> > but one question before... Is this related to NLA_UINT in the kernel?
>> > > >> > 
>> > > >> > /**
>> > > >> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
>> > > >> >  * @skb: socket buffer to add attribute to
>> > > >> >  * @attrtype: attribute type
>> > > >> >  * @value: numeric value
>> > > >> >  */
>> > > >> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
>> > > >> > {
>> > > >> >         u64 tmp64 = value;
>> > > >> >         u32 tmp32 = value;
>> > > >> > 
>> > > >> >         if (tmp64 == tmp32)
>> > > >> >                 return nla_put_u32(skb, attrtype, tmp32);
>> > > >> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
>> > > >> > }
>> > > >> > 
>> > > >> > if I'm correct, it seems kernel always uses either u32 or u64.
>> > > >> > 
>> > > >> > Userspace assumes u8 and u16 are possible though:
>> > > >> > 
>> > > >> > +/**
>> > > >> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
>> > > >> > + * \param attr pointer to netlink attribute
>> > > >> > + *
>> > > >> > + * This function returns the 64-bit value of the attribute payload.
>> > > >> > + */
>> > > >> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
>> > > >> > +{
>> > > >> > +       switch (mnl_attr_get_payload_len(attr)) {
>> > > >> > +       case sizeof(uint8_t):
>> > > >> > +               return mnl_attr_get_u8(attr);
>> > > >> > +       case sizeof(uint16_t):
>> > > >> > +               return mnl_attr_get_u16(attr);
>> > > >> > +       case sizeof(uint32_t):
>> > > >> > +               return mnl_attr_get_u32(attr);
>> > > >> > +       case sizeof(uint64_t):
>> > > >> > +               return mnl_attr_get_u64(attr);
>> > > >> > +       }
>> > > >> > +
>> > > >> > +       return -1ULL;
>> > > >> > +}
>> > > >> > 
>> > > >> > Or this is an attempt to provide a helper that allows you fetch for
>> > > >> > payload value of 2^3..2^6 bytes?
>> > > >> 
>> > > >> No preference here, FWIW. Looks like this patch does a different thing
>> > > >> than the kernel. But maybe a broader "automatic" helper is useful for
>> > > >> user space code.
>> > > >
>> > > > Not sure. @Danielle: could you clarify your intention?
>> > > 
>> > > This follows the iproute2 helper, where I was asked to support >32-bit
>> > > fields purely as a service to the users, so that one helper can be used
>> > > for any integral field.
>> > 
>> > Which helper are your referring to? Is it modeled after NLA_UINT?
>> > 
>> > I don't think this patch is fine. This also returns -1ULL so there is
>> > no way to know if size is not correct or payload length is 64 bits
>> > using UINT64_MAX?
>> 
>> I found it:
>> 
>> static inline __u64 rta_getattr_uint(const struct rtattr *rta)
>> 
>> This only has one user in the tree so far, right?
>
> Well, this is a matter of documenting behaviour.
>
>> include/libnetlink.h:static inline __u64 rta_getattr_uint(const struct rtattr *rta)
>> ip/ipnexthop.c:                 nh_grp_stats->packets = rta_getattr_uint(rta);
>> ip/ipnexthop.c:                 nh_grp_stats->packets_hw = rta_getattr_uint(rta);
>> 
>> is this attribute for ipnexthop of NLA_UINT type?
>
> But it seems intention is to support NLA_UINT according to iproute's
> commit.
>
> commit 95836fbf35d352f7c031ddac2e6093a935308cc9
> Author: Petr Machata <petrm@nvidia.com>
> Date:   Thu Mar 14 15:52:12 2024 +0100
>
>     libnetlink: Add rta_getattr_uint()
>     
>     NLA_UINT attributes have a 4-byte payload if possible, and an 8-byte one if
>     necessary. Add a function to extract these. Since we need to dispatch on
>     length anyway, make the getter truly universal by supporting also u8 and
>     u16.
>
> so it went further to make it universal for 2^3..2^6 values.
>
> I am going to submit a patch to provide more info on this helper function.

Yeah, sorry, I wrote this to explain the rationale:

> This follows the iproute2 helper, where I was asked to support >32-bit
> fields purely as a service to the users, so that one helper can be used
> for any integral field.

... but of course I should have written <32-bit payloads, not >32-bit.

Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Petr Machata]

Pablo Neira Ayuso <pablo@netfilter.org> writes:

> On Mon, Sep 30, 2024 at 07:01:42PM +0200, Pablo Neira Ayuso wrote:
>> On Mon, Sep 30, 2024 at 06:25:17PM +0200, Petr Machata wrote:
>> > 
>> > Pablo Neira Ayuso <pablo@netfilter.org> writes:
>> > 
>> > > On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
>> > >> On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
>> > >> > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
>> > >> > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:  
>> > >> > > > Hi,
>> > >> > > > 
>> > >> > > > Is there a plan to build a new version soon? 
>> > >> > > > I am asking since I am planning to use this function in ethtool.  
>> > >> > > 
>> > >> > > ASAP  
>> > >> > 
>> > >> > but one question before... Is this related to NLA_UINT in the kernel?
>> > >> > 
>> > >> > /**
>> > >> >  * nla_put_uint - Add a variable-size unsigned int to a socket buffer
>> > >> >  * @skb: socket buffer to add attribute to
>> > >> >  * @attrtype: attribute type
>> > >> >  * @value: numeric value
>> > >> >  */
>> > >> > static inline int nla_put_uint(struct sk_buff *skb, int attrtype, u64 value)
>> > >> > {
>> > >> >         u64 tmp64 = value;
>> > >> >         u32 tmp32 = value;
>> > >> > 
>> > >> >         if (tmp64 == tmp32)
>> > >> >                 return nla_put_u32(skb, attrtype, tmp32);
>> > >> >         return nla_put(skb, attrtype, sizeof(u64), &tmp64);
>> > >> > }
>> > >> > 
>> > >> > if I'm correct, it seems kernel always uses either u32 or u64.
>> > >> > 
>> > >> > Userspace assumes u8 and u16 are possible though:
>> > >> > 
>> > >> > +/**
>> > >> > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
>> > >> > + * \param attr pointer to netlink attribute
>> > >> > + *
>> > >> > + * This function returns the 64-bit value of the attribute payload.
>> > >> > + */
>> > >> > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
>> > >> > +{
>> > >> > +       switch (mnl_attr_get_payload_len(attr)) {
>> > >> > +       case sizeof(uint8_t):
>> > >> > +               return mnl_attr_get_u8(attr);
>> > >> > +       case sizeof(uint16_t):
>> > >> > +               return mnl_attr_get_u16(attr);
>> > >> > +       case sizeof(uint32_t):
>> > >> > +               return mnl_attr_get_u32(attr);
>> > >> > +       case sizeof(uint64_t):
>> > >> > +               return mnl_attr_get_u64(attr);
>> > >> > +       }
>> > >> > +
>> > >> > +       return -1ULL;
>> > >> > +}
>> > >> > 
>> > >> > Or this is an attempt to provide a helper that allows you fetch for
>> > >> > payload value of 2^3..2^6 bytes?
>> > >> 
>> > >> No preference here, FWIW. Looks like this patch does a different thing
>> > >> than the kernel. But maybe a broader "automatic" helper is useful for
>> > >> user space code.
>> > >
>> > > Not sure. @Danielle: could you clarify your intention?
>> > 
>> > This follows the iproute2 helper, where I was asked to support >32-bit
>> > fields purely as a service to the users, so that one helper can be used
>> > for any integral field.
>> 
>> Which helper are your referring to? Is it modeled after NLA_UINT?
>> 
>> I don't think this patch is fine. This also returns -1ULL so there is
>> no way to know if size is not correct or payload length is 64 bits
>> using UINT64_MAX?

That's no different from the other mnl_attr_get_uX() helpers that just
assume the user has passed in the right attribute type.

> I found it:
>
> static inline __u64 rta_getattr_uint(const struct rtattr *rta)

That's the one.

> This only has one user in the tree so far, right?

Well, two, but yeah, the u8 / u16 stuff is not currently used.

> include/libnetlink.h:static inline __u64 rta_getattr_uint(const struct rtattr *rta)
> ip/ipnexthop.c:                 nh_grp_stats->packets = rta_getattr_uint(rta);
> ip/ipnexthop.c:                 nh_grp_stats->packets_hw = rta_getattr_uint(rta);
>
> is this attribute for ipnexthop of NLA_UINT type?

Yeah, both NHA_GROUP_STATS_ENTRY_PACKETS and
NHA_GROUP_STATS_ENTRY_PACKETS_HW are NLA_UINT.

RE: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function

[Danielle Ratson]

> From: Pablo Neira Ayuso <pablo@netfilter.org>
> Sent: Monday, 30 September 2024 15:48
> To: Jakub Kicinski <kuba@kernel.org>
> Cc: Danielle Ratson <danieller@nvidia.com>; Phil Sutter <phil@nwl.cc>;
> netfilter-devel@vger.kernel.org; fw@strlen.de; mlxsw <mlxsw@nvidia.com>
> Subject: Re: [PATCH libmnl] src: attr: Add mnl_attr_get_uint() function
> 
> On Mon, Sep 30, 2024 at 01:45:09PM +0200, Jakub Kicinski wrote:
> > On Mon, 30 Sep 2024 12:56:20 +0200 Pablo Neira Ayuso wrote:
> > > On Mon, Sep 30, 2024 at 12:28:08PM +0200, Pablo Neira Ayuso wrote:
> > > > On Sun, Sep 29, 2024 at 10:42:44AM +0000, Danielle Ratson wrote:
> > > > > Hi,
> > > > >
> > > > > Is there a plan to build a new version soon?
> > > > > I am asking since I am planning to use this function in ethtool.
> > > >
> > > > ASAP
> > >
> > > but one question before... Is this related to NLA_UINT in the kernel?
> > >
> > > /**
> > >  * nla_put_uint - Add a variable-size unsigned int to a socket
> > > buffer
> > >  * @skb: socket buffer to add attribute to
> > >  * @attrtype: attribute type
> > >  * @value: numeric value
> > >  */
> > > static inline int nla_put_uint(struct sk_buff *skb, int attrtype,
> > > u64 value) {
> > >         u64 tmp64 = value;
> > >         u32 tmp32 = value;
> > >
> > >         if (tmp64 == tmp32)
> > >                 return nla_put_u32(skb, attrtype, tmp32);
> > >         return nla_put(skb, attrtype, sizeof(u64), &tmp64); }
> > >
> > > if I'm correct, it seems kernel always uses either u32 or u64.
> > >
> > > Userspace assumes u8 and u16 are possible though:
> > >
> > > +/**
> > > + * mnl_attr_get_uint - returns 64-bit unsigned integer attribute.
> > > + * \param attr pointer to netlink attribute
> > > + *
> > > + * This function returns the 64-bit value of the attribute payload.
> > > + */
> > > +EXPORT_SYMBOL uint64_t mnl_attr_get_uint(const struct nlattr *attr)
> > > +{
> > > +       switch (mnl_attr_get_payload_len(attr)) {
> > > +       case sizeof(uint8_t):
> > > +               return mnl_attr_get_u8(attr);
> > > +       case sizeof(uint16_t):
> > > +               return mnl_attr_get_u16(attr);
> > > +       case sizeof(uint32_t):
> > > +               return mnl_attr_get_u32(attr);
> > > +       case sizeof(uint64_t):
> > > +               return mnl_attr_get_u64(attr);
> > > +       }
> > > +
> > > +       return -1ULL;
> > > +}
> > >
> > > Or this is an attempt to provide a helper that allows you fetch for
> > > payload value of 2^3..2^6 bytes?
> >
> > No preference here, FWIW. Looks like this patch does a different thing
> > than the kernel. But maybe a broader "automatic" helper is useful for
> > user space code.
> 
> Not sure. @Danielle: could you clarify your intention?

Hi,

Thanks for all the comments.

As I see it, there are at least two occurrences in ethtool for a code that tries to overcome the lack of this kind of uint helper when the uint attribute type exists in the kernel and seems like more recommended to use then before.

Therefore, as I see it, this helper seems more reasonable to have.

But I am trying to understand, are we discussing the need of the patch at all? Or some nits about the way it is implemented? Or something else?
I am asking since the patch was already taken by @Pablo Neira Ayuso.

> 
> If this is to support NLA_UINT, I'd prefer to stick to NLA_UINT semantics.
> 
> @Jakub: is there any plan to augment NLA_UINT in the future? What the
> assumption from userspace that this will always return 32-bits else 64-bits
> value?
> 
> Thanks.