From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.2 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A9257ECE58C for ; Fri, 11 Oct 2019 13:34:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 608F92084C for ; Fri, 11 Oct 2019 13:34:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=resnulli-us.20150623.gappssmtp.com header.i=@resnulli-us.20150623.gappssmtp.com header.b="zAskxPje" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728442AbfJKNeh (ORCPT ); Fri, 11 Oct 2019 09:34:37 -0400 Received: from mail-wr1-f67.google.com ([209.85.221.67]:37987 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728231AbfJKNeg (ORCPT ); Fri, 11 Oct 2019 09:34:36 -0400 Received: by mail-wr1-f67.google.com with SMTP id y18so2506070wrn.5 for ; Fri, 11 Oct 2019 06:34:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=resnulli-us.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=8GDzeyDx7lHaxA8YP8ygv20c48t58ZZ1n1DBbljGJkU=; b=zAskxPjevfioGiTfIMS6lnfJBKGYQ30zG9luDA8vKor7N5Rb0XcqhnqjdKraEuAKa3 secLE6kujCDq9jV+knPWrM4eXIsqHJMatrXHjDSqp4xSjRUTh5HIM68atAi9D31Mztvx dd5331DlOb1pncuIan+M4qJN7OS+I8EtgHD1hwhqXdipUL/sbjE4IghkRFwJ/cMrPX9x EoyJSr22EUc61RuKqNOiFRlu28zbi4rkw/R8kQ0rvLtDM+JdnR5v025oCcy0kdOGaK49 bmB6bbh/SfUnMxfQsna01TiTJg/VhlBp2TqREFRGxzlhN2OO/MQtNNS4uT2U5DjBXLD7 et8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=8GDzeyDx7lHaxA8YP8ygv20c48t58ZZ1n1DBbljGJkU=; b=MGFSqnOwadxOU+Kn4bb3UylzT11/FyixBnztVaPQDclBQvaxG1CRTFOWKWFzFirUpa w+7qyq4qQtUNfteO3QAYJRoMu1qbHkQ+8b048ypnFLFCve9HQ9d/5exh1oocuE6aFmC2 fAnnAv0L/3cL4msXezDpyD/8hghD461VJadQWVEWkuIlAk1c7a7cc4+H8RQh1spDyamf Jn11BucvvIo8/WEhlZF/zWwPg7nPwiMRSQvuSbDfDSf/GZF8Pv7AKSCnIYwYpXpymnIg z/79e3z1OW35zPmQzCOddpKxK+0lL3OkuG0peNpHl34iuEObQW7LFzVWTzmneUujqCzE MwRA== X-Gm-Message-State: APjAAAU7MkSwakq5786Wq5yDX5dKg1ruh6/wkf5+7iWR8jvDXwIavNaj LPdLevZ12e597bJojktTxyUXGA== X-Google-Smtp-Source: APXvYqwmL1RqsQt+9UZmSKuOA1E+GsbE2rMfSeUgruR1CEQWGd1ng49mcv5YtckneSBl0qzWOv787w== X-Received: by 2002:adf:ec84:: with SMTP id z4mr13275862wrn.254.1570800870930; Fri, 11 Oct 2019 06:34:30 -0700 (PDT) Received: from localhost ([85.163.43.78]) by smtp.gmail.com with ESMTPSA id q124sm16133313wma.5.2019.10.11.06.34.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Oct 2019 06:34:30 -0700 (PDT) Date: Fri, 11 Oct 2019 15:34:29 +0200 From: Jiri Pirko To: Michal Kubecek Cc: David Miller , netdev@vger.kernel.org, Jakub Kicinski , Andrew Lunn , Florian Fainelli , John Linville , Stephen Hemminger , Johannes Berg , linux-kernel@vger.kernel.org Subject: Re: [PATCH net-next v7 06/17] ethtool: netlink bitset handling Message-ID: <20191011133429.GA3056@nanopsycho> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.4 (2019-03-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org Wed, Oct 09, 2019 at 10:59:18PM CEST, mkubecek@suse.cz wrote: >The ethtool netlink code uses common framework for passing arbitrary >length bit sets to allow future extensions. A bitset can be a list (only >one bitmap) or can consist of value and mask pair (used e.g. when client >want to modify only some bits). A bitset can use one of two formats: >verbose (bit by bit) or compact. > >Verbose format consists of bitset size (number of bits), list flag and >an array of bit nests, telling which bits are part of the list or which >bits are in the mask and which of them are to be set. In requests, bits >can be identified by index (position) or by name. In replies, kernel >provides both index and name. Verbose format is suitable for "one shot" >applications like standard ethtool command as it avoids the need to >either keep bit names (e.g. link modes) in sync with kernel or having to >add an extra roundtrip for string set request (e.g. for private flags). > >Compact format uses one (list) or two (value/mask) arrays of 32-bit >words to store the bitmap(s). It is more suitable for long running >applications (ethtool in monitor mode or network management daemons) >which can retrieve the names once and then pass only compact bitmaps to >save space. > >Userspace requests can use either format; ETHTOOL_GFLAG_COMPACT_BITSETS >flag in request header tells kernel which format to use in reply. >Notifications always use compact format. > >Signed-off-by: Michal Kubecek >--- > Documentation/networking/ethtool-netlink.rst | 68 ++ > include/uapi/linux/ethtool_netlink.h | 35 + > net/ethtool/Makefile | 2 +- > net/ethtool/bitset.c | 714 +++++++++++++++++++ > net/ethtool/bitset.h | 28 + > net/ethtool/netlink.h | 9 + > 6 files changed, 855 insertions(+), 1 deletion(-) > create mode 100644 net/ethtool/bitset.c > create mode 100644 net/ethtool/bitset.h > >diff --git a/Documentation/networking/ethtool-netlink.rst b/Documentation/networking/ethtool-netlink.rst >index 3e9680b63afa..8dda6efee060 100644 >--- a/Documentation/networking/ethtool-netlink.rst >+++ b/Documentation/networking/ethtool-netlink.rst >@@ -79,6 +79,74 @@ clients not aware of the flag should be interpreted the way the client > expects. A client must not set flags it does not understand. > > >+Bit sets >+======== >+ >+For short bitmaps of (reasonably) fixed length, standard ``NLA_BITFIELD32`` >+type is used. For arbitrary length bitmaps, ethtool netlink uses a nested >+attribute with contents of one of two forms: compact (two binary bitmaps >+representing bit values and mask of affected bits) and bit-by-bit (list of >+bits identified by either index or name). >+ >+Compact form: nested (bitset) atrribute contents: >+ >+ ============================ ====== ============================ >+ ``ETHTOOL_A_BITSET_LIST`` flag no mask, only a list I find "list" a bit confusing name of a flag. Perhaps better to stick with the "compact" terminology and make this "ETHTOOL_A_BITSET_COMPACT"? Then in the code you can have var "is_compact", which makes the code a bit easier to read I believe. >+ ``ETHTOOL_A_BITSET_SIZE`` u32 number of significant bits >+ ``ETHTOOL_A_BITSET_VALUE`` binary bitmap of bit values >+ ``ETHTOOL_A_BITSET_MASK`` binary bitmap of valid bits Couple of times the NLA_BITFIELD32 limitation was discussed, so isn't this the time to introduce generic NLA_BITFIELD with variable len and use it here? This is exactly job for it. As this is UAPI, I believe it should be done now cause later won't work. >+ ============================ ====== ============================ >+ >+Value and mask must have length at least ``ETHTOOL_A_BITSET_SIZE`` bits >+rounded up to a multiple of 32 bits. They consist of 32-bit words in host byte >+order, words ordered from least significant to most significant (i.e. the same >+way as bitmaps are passed with ioctl interface). >+ >+For compact form, ``ETHTOOL_A_BITSET_SIZE`` and ``ETHTOOL_A_BITSET_VALUE`` are >+mandatory. Similar to ``NLA_BITFIELD32``, a compact form bit set requests to >+set bits in the mask to 1 (if the bit is set in value) or 0 (if not) and >+preserve the rest. If ``ETHTOOL_A_BITSET_LIST`` is present, there is no mask >+and bitset represents a simple list of bits. >+ >+Kernel bit set length may differ from userspace length if older application is >+used on newer kernel or vice versa. If userspace bitmap is longer, an error is >+issued only if the request actually tries to set values of some bits not >+recognized by kernel. >+ >+Bit-by-bit form: nested (bitset) attribute contents: >+ >+ +---------------------------------+--------+-----------------------------+ >+ | ``ETHTOOL_A_BITSET_LIST`` | flag | no mask, only a list | >+ +---------------------------------+--------+-----------------------------+ >+ | ``ETHTOOL_A_BITSET_SIZE`` | u32 | number of significant bits | >+ +---------------------------------+--------+-----------------------------+ >+ | ``ETHTOOL_A_BITSET_BIT`` | nested | array of bits | "ETHTOOL_A_BITSET_BIT" does not exist in the code. I believe you ment "ETHTOOL_A_BITSET_BITS" >+ +-+-------------------------------+--------+-----------------------------+ >+ | ``ETHTOOL_A_BITSET_BIT+`` | nested | one bit | You seem to be missing "|" here. Also "ETHTOOL_A_BITSET_BIT" does not exist. I believe you ment "ETHTOOL_A_BITS_BIT" >+ +-+-+-----------------------------+--------+-----------------------------+ >+ | | | ``ETHTOOL_A_BIT_INDEX`` | u32 | bit index (0 for LSB) | >+ +-+-+-----------------------------+--------+-----------------------------+ >+ | | | ``ETHTOOL_A_BIT_NAME`` | string | bit name | >+ +-+-+-----------------------------+--------+-----------------------------+ >+ | | | ``ETHTOOL_A_BIT_VALUE`` | flag | present if bit is set | >+ +-+-+-----------------------------+--------+-----------------------------+ >+ >+Bit size is optional for bit-by-bit form. ``ETHTOOL_A_BITSET_BITS`` nest can >+only contain ``ETHTOOL_A_BITS_BIT`` attributes but there can be an arbitrary >+number of them. A bit may be identified by its index or by its name. When >+used in requests, listed bits are set to 0 or 1 according to >+``ETHTOOL_A_BIT_VALUE``, the rest is preserved. A request fails if index >+exceeds kernel bit length or if name is not recognized. >+ >+When ``ETHTOOL_A_BITSET_LIST`` flag is present, bitset is interpreted as a >+simple bit list. ``ETHTOOL_A_BIT_VALUE`` attributes are not used in such case. >+Bit list represents a bitmap with listed bits set and the rest zero. >+ >+In requests, application can use either form. Form used by kernel in reply is >+determined by a flag in flags field of request header. Semantics of value and >+mask depends on the attribute. >+ >+ > List of message types > ===================== > >diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h >index c58d9fd52ffc..418f28965a04 100644 >--- a/include/uapi/linux/ethtool_netlink.h >+++ b/include/uapi/linux/ethtool_netlink.h >@@ -51,6 +51,41 @@ enum { > ETHTOOL_A_HEADER_MAX = __ETHTOOL_A_HEADER_CNT - 1 > }; > >+/* bit sets */ >+ >+enum { >+ ETHTOOL_A_BIT_UNSPEC, >+ ETHTOOL_A_BIT_INDEX, /* u32 */ >+ ETHTOOL_A_BIT_NAME, /* string */ >+ ETHTOOL_A_BIT_VALUE, /* flag */ >+ >+ /* add new constants above here */ >+ __ETHTOOL_A_BIT_CNT, >+ ETHTOOL_A_BIT_MAX = __ETHTOOL_A_BIT_CNT - 1 >+}; >+ >+enum { >+ ETHTOOL_A_BITS_UNSPEC, >+ ETHTOOL_A_BITS_BIT, >+ >+ /* add new constants above here */ >+ __ETHTOOL_A_BITS_CNT, >+ ETHTOOL_A_BITS_MAX = __ETHTOOL_A_BITS_CNT - 1 >+}; I think it would be good to have this named with "_BITSET_" in it so it is crystal clear this is part of the bitset UAPI. >+ >+enum { >+ ETHTOOL_A_BITSET_UNSPEC, >+ ETHTOOL_A_BITSET_LIST, /* flag */ >+ ETHTOOL_A_BITSET_SIZE, /* u32 */ >+ ETHTOOL_A_BITSET_BITS, /* nest - _A_BITS_* */ >+ ETHTOOL_A_BITSET_VALUE, /* binary */ >+ ETHTOOL_A_BITSET_MASK, /* binary */ >+ >+ /* add new constants above here */ >+ __ETHTOOL_A_BITSET_CNT, >+ ETHTOOL_A_BITSET_MAX = __ETHTOOL_A_BITSET_CNT - 1 >+}; >+ > /* generic netlink info */ > #define ETHTOOL_GENL_NAME "ethtool" > #define ETHTOOL_GENL_VERSION 1 >diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile >index f30e0da88be5..482fdb9380fa 100644 >--- a/net/ethtool/Makefile >+++ b/net/ethtool/Makefile >@@ -4,4 +4,4 @@ obj-y += ioctl.o > > obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o > >-ethtool_nl-y := netlink.o >+ethtool_nl-y := netlink.o bitset.o >diff --git a/net/ethtool/bitset.c b/net/ethtool/bitset.c >new file mode 100644 >index 000000000000..aff6413d6bcc >--- /dev/null >+++ b/net/ethtool/bitset.c >@@ -0,0 +1,714 @@ >+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note >+ >+#include >+#include >+#include "netlink.h" >+#include "bitset.h" >+ >+/* To reduce the number of slab allocations, the wrappers use fixed size local >+ * variables for bitmaps up to __SMALL_BITMAP_BITS bits which is the majority >+ * of bitmaps used by ethtool. >+ */ >+#define __SMALL_BITMAP_BITS 128 >+#define __SMALL_BITMAP_WORDS DIV_ROUND_UP(__SMALL_BITMAP_BITS, 32) >+ >+static u32 __lower_bits(unsigned int n) >+{ >+ return ~(u32)0 >> (32 - n % 32); >+} >+ >+static u32 __upper_bits(unsigned int n) >+{ >+ return ~(u32)0 << (n % 32); >+} >+ >+/** >+ * __bitmap32_clear() - Clear u32 based bitmap >+ * @dst: bitmap to clear >+ * @start: beginning of the interval >+ * @end: end of the interval >+ * @mod: set if bitmap was modified >+ * >+ * Clear @nbits bits of a bitmap with indices @start <= i < @end >+ */ >+static void __bitmap32_clear(u32 *dst, unsigned int start, unsigned int end, >+ bool *mod) >+{ >+ unsigned int start_word = start / 32; >+ unsigned int end_word = end / 32; >+ unsigned int i; >+ u32 mask; >+ >+ if (end <= start) >+ return; >+ >+ if (start % 32) { >+ mask = __upper_bits(start); >+ if (end_word == start_word) { >+ mask &= __lower_bits(end); >+ if (dst[start_word] & mask) { >+ dst[start_word] &= ~mask; >+ *mod = true; >+ } >+ return; >+ } >+ if (dst[start_word] & mask) { >+ dst[start_word] &= ~mask; >+ *mod = true; >+ } >+ start_word++; >+ } >+ >+ for (i = start_word; i < end_word; i++) { >+ if (dst[i]) { >+ dst[i] = 0; >+ *mod = true; >+ } >+ } >+ if (end % 32) { >+ mask = __lower_bits(end); >+ if (dst[end_word] & mask) { >+ dst[end_word] &= ~mask; >+ *mod = true; >+ } >+ } >+} >+ >+/** >+ * __bitmap32_no_zero() - Check if any bit is set in an interval >+ * @map: bitmap to test >+ * @start: beginning of the interval >+ * @end: end of the interval >+ * >+ * Return: true if there is non-zero bit with index @start <= i < @end, >+ * false if the whole interval is zero >+ */ >+static bool __bitmap32_not_zero(const u32 *map, unsigned int start, >+ unsigned int end) >+{ >+ unsigned int start_word = start / 32; >+ unsigned int end_word = end / 32; >+ u32 mask; >+ >+ if (end <= start) >+ return true; >+ >+ if (start % 32) { >+ mask = __upper_bits(start); >+ if (end_word == start_word) { >+ mask &= __lower_bits(end); >+ return map[start_word] & mask; >+ } >+ if (map[start_word] & mask) >+ return true; >+ start_word++; >+ } >+ >+ if (!memchr_inv(map + start_word, '\0', >+ (end_word - start_word) * sizeof(u32))) >+ return true; >+ if (end % 32 == 0) >+ return true; >+ return map[end_word] & __lower_bits(end); >+} >+ >+/** >+ * __bitmap32_update() - Modify u32 based bitmap according to value/mask pair >+ * @dst: bitmap to update >+ * @nbits: bit size of the bitmap >+ * @value: values to set >+ * @mask: mask of bits to set >+ * @mod: set to true if bitmap is modified, preserve if not >+ * >+ * Set bits in @dst bitmap which are set in @mask to values from @value, leave >+ * the rest untouched. If destination bitmap was modified, set @mod to true, >+ * leave as it is if not. >+ */ >+static void __bitmap32_update(u32 *dst, unsigned int nbits, const u32 *value, >+ const u32 *mask, bool *mod) >+{ >+ while (nbits > 0) { >+ u32 real_mask = mask ? *mask : ~(u32)0; >+ u32 new_value; >+ >+ if (nbits < 32) >+ real_mask &= __lower_bits(nbits); >+ new_value = (*dst & ~real_mask) | (*value & real_mask); >+ if (new_value != *dst) { >+ *dst = new_value; >+ *mod = true; >+ } >+ >+ if (nbits <= 32) >+ break; >+ dst++; >+ nbits -= 32; >+ value++; >+ if (mask) >+ mask++; >+ } >+} >+ >+static bool __bitmap32_test_bit(const u32 *map, unsigned int index) >+{ >+ return map[index / 32] & (1U << (index % 32)); >+} >+ >+/** >+ * ethnl_bitset32_size() - Calculate size of bitset nested attribute >+ * @val: value bitmap (u32 based) >+ * @mask: mask bitmap (u32 based, optional) >+ * @nbits: bit length of the bitset >+ * @names: array of bit names (optional) >+ * @compact: assume compact format for output >+ * >+ * Estimate length of netlink attribute composed by a later call to >+ * ethnl_put_bitset32() call with the same arguments. >+ * >+ * Return: negative error code or attribute length estimate >+ */ >+int ethnl_bitset32_size(const u32 *val, const u32 *mask, unsigned int nbits, >+ ethnl_string_array_t names, bool compact) >+{ >+ unsigned int len = 0; >+ >+ /* list flag */ >+ if (!mask) >+ len += nla_total_size(sizeof(u32)); >+ /* size */ >+ len += nla_total_size(sizeof(u32)); >+ >+ if (compact) { >+ unsigned int nwords = DIV_ROUND_UP(nbits, 32); >+ >+ /* value, mask */ >+ len += (mask ? 2 : 1) * nla_total_size(nwords * sizeof(u32)); >+ } else { >+ unsigned int bits_len = 0; >+ unsigned int bit_len, i; >+ >+ for (i = 0; i < nbits; i++) { >+ const char *name = names ? names[i] : NULL; >+ >+ if (!__bitmap32_test_bit(mask ?: val, i)) >+ continue; >+ /* index */ >+ bit_len = nla_total_size(sizeof(u32)); >+ /* name */ >+ if (name) >+ bit_len += ethnl_strz_size(name); >+ /* value */ >+ if (mask && __bitmap32_test_bit(val, i)) >+ bit_len += nla_total_size(0); >+ >+ /* bit nest */ >+ bits_len += nla_total_size(bit_len); >+ } >+ /* bits nest */ >+ len += nla_total_size(bits_len); >+ } >+ >+ /* outermost nest */ >+ return nla_total_size(len); >+} >+ >+/** >+ * ethnl_put_bitset32() - Put a bitset nest into a message >+ * @skb: skb with the message >+ * @attrtype: attribute type for the bitset nest >+ * @val: value bitmap (u32 based) >+ * @mask: mask bitmap (u32 based, optional) >+ * @nbits: bit length of the bitset >+ * @names: array of bit names (optional) >+ * @compact: use compact format for the output >+ * >+ * Compose a nested attribute representing a bitset. If @mask is null, simple >+ * bitmap (bit list) is created, if @mask is provided, represent a value/mask >+ * pair. Bit names are only used in verbose mode and when provided by calller. >+ * >+ * Return: 0 on success, negative error value on error Remove the spaces. >+ */ >+int ethnl_put_bitset32(struct sk_buff *skb, int attrtype, const u32 *val, >+ const u32 *mask, unsigned int nbits, >+ ethnl_string_array_t names, bool compact) >+{ >+ struct nlattr *nest; >+ struct nlattr *attr; >+ >+ nest = nla_nest_start(skb, attrtype); >+ if (!nest) >+ return -EMSGSIZE; >+ >+ if (!mask && nla_put_flag(skb, ETHTOOL_A_BITSET_LIST)) Wait, shouldn't you rather check "!compact" ? and WARN_ON in case compact == true && mask == NULL? >+ goto nla_put_failure; >+ if (nla_put_u32(skb, ETHTOOL_A_BITSET_SIZE, nbits)) >+ goto nla_put_failure; >+ if (compact) { >+ unsigned int nwords = DIV_ROUND_UP(nbits, 32); >+ unsigned int nbytes = nwords * sizeof(u32); >+ u32 *dst; >+ >+ attr = nla_reserve(skb, ETHTOOL_A_BITSET_VALUE, nbytes); >+ if (!attr) >+ goto nla_put_failure; >+ dst = nla_data(attr); >+ memcpy(dst, val, nbytes); >+ if (nbits % 32) >+ dst[nwords - 1] &= __lower_bits(nbits); >+ >+ if (mask) { >+ attr = nla_reserve(skb, ETHTOOL_A_BITSET_MASK, nbytes); >+ if (!attr) >+ goto nla_put_failure; >+ dst = nla_data(attr); >+ memcpy(dst, mask, nbytes); >+ if (nbits % 32) >+ dst[nwords - 1] &= __lower_bits(nbits); >+ } >+ } else { >+ struct nlattr *bits; >+ unsigned int i; >+ >+ bits = nla_nest_start(skb, ETHTOOL_A_BITSET_BITS); >+ if (!bits) >+ goto nla_put_failure; >+ for (i = 0; i < nbits; i++) { >+ const char *name = names ? names[i] : NULL; >+ >+ if (!__bitmap32_test_bit(mask ?: val, i)) A) this __bitmap32_test_bit() looks like something generic, yet it is not. Perhaps you would want to add this helper to include/linux/bitmap.h? B) Why don't you do bitmap_to_arr32 conversion in this function just before val/mask put. Then you can use normal test_bit() here. >+ continue; >+ attr = nla_nest_start(skb, ETHTOOL_A_BITS_BIT); >+ if (!attr || >+ nla_put_u32(skb, ETHTOOL_A_BIT_INDEX, i)) You mix these 2 in 1 if which are not related. Better keep them separate in two ifs. Or you can put the rest of the puts in the same if too. >+ goto nla_put_failure; >+ if (name && >+ ethnl_put_strz(skb, ETHTOOL_A_BIT_NAME, name)) >+ goto nla_put_failure; >+ if (mask && __bitmap32_test_bit(val, i) && >+ nla_put_flag(skb, ETHTOOL_A_BIT_VALUE)) >+ goto nla_put_failure; >+ nla_nest_end(skb, attr); >+ } >+ nla_nest_end(skb, bits); >+ } >+ >+ nla_nest_end(skb, nest); >+ return 0; >+ >+nla_put_failure: >+ nla_nest_cancel(skb, nest); >+ return -EMSGSIZE; >+} >+ >+static const struct nla_policy bitset_policy[ETHTOOL_A_BITSET_MAX + 1] = { >+ [ETHTOOL_A_BITSET_UNSPEC] = { .type = NLA_REJECT }, >+ [ETHTOOL_A_BITSET_LIST] = { .type = NLA_FLAG }, >+ [ETHTOOL_A_BITSET_SIZE] = { .type = NLA_U32 }, >+ [ETHTOOL_A_BITSET_BITS] = { .type = NLA_NESTED }, >+ [ETHTOOL_A_BITSET_VALUE] = { .type = NLA_BINARY }, >+ [ETHTOOL_A_BITSET_MASK] = { .type = NLA_BINARY }, >+}; >+ >+static const struct nla_policy bit_policy[ETHTOOL_A_BIT_MAX + 1] = { >+ [ETHTOOL_A_BIT_UNSPEC] = { .type = NLA_REJECT }, >+ [ETHTOOL_A_BIT_INDEX] = { .type = NLA_U32 }, >+ [ETHTOOL_A_BIT_NAME] = { .type = NLA_NUL_STRING }, >+ [ETHTOOL_A_BIT_VALUE] = { .type = NLA_FLAG }, >+}; >+ >+/** >+ * ethnl_bitset_is_compact() - check if bitset attribute represents a compact >+ * bitset >+ * @bitset - nested attribute representing a bitset >+ * @compact - pointer for return value In the rest of the code, you use @name: description >+ * >+ * Return: 0 on success, negative error code on failure >+ */ >+int ethnl_bitset_is_compact(const struct nlattr *bitset, bool *compact) >+{ >+ struct nlattr *tb[ETHTOOL_A_BITSET_MAX + 1]; >+ int ret; >+ >+ ret = nla_parse_nested(tb, ETHTOOL_A_BITSET_MAX, bitset, >+ bitset_policy, NULL); >+ if (ret < 0) >+ return ret; >+ >+ if (tb[ETHTOOL_A_BITSET_BITS]) { >+ if (tb[ETHTOOL_A_BITSET_VALUE] || tb[ETHTOOL_A_BITSET_MASK]) >+ return -EINVAL; >+ *compact = false; >+ return 0; >+ } >+ if (!tb[ETHTOOL_A_BITSET_SIZE] || !tb[ETHTOOL_A_BITSET_VALUE]) >+ return -EINVAL; >+ >+ *compact = true; >+ return 0; >+} >+ >+static int ethnl_name_to_idx(ethnl_string_array_t names, unsigned int n_names, >+ const char *name, unsigned int name_len) >+{ >+ unsigned int i; >+ >+ if (!names) >+ return n_names; >+ >+ for (i = 0; i < n_names; i++) { >+ const char *bname = names[i]; >+ >+ if (!strncmp(bname, name, name_len) && >+ strlen(bname) <= name_len) >+ return i; >+ } >+ >+ return n_names; Maybe bettet to stick with -ERRNO? >+} >+ >+static int ethnl_parse_bit(unsigned int *index, bool *val, unsigned int nbits, >+ const struct nlattr *bit_attr, bool is_list, >+ ethnl_string_array_t names, >+ struct netlink_ext_ack *extack) >+{ >+ struct nlattr *tb[ETHTOOL_A_BIT_MAX + 1]; >+ int ret, idx; >+ >+ if (nla_type(bit_attr) != ETHTOOL_A_BITS_BIT) { >+ NL_SET_ERR_MSG_ATTR(extack, bit_attr, >+ "only ETHTOOL_A_BITS_BIT allowed in ETHTOOL_A_BITSET_BITS"); >+ return -EINVAL; >+ } Probably it makes sense the caller does this check. Later on, if there is another possible value, the check would have to go there anyway. >+ ret = nla_parse_nested(tb, ETHTOOL_A_BIT_MAX, bit_attr, bit_policy, >+ extack); >+ if (ret < 0) >+ return ret; >+ >+ if (tb[ETHTOOL_A_BIT_INDEX]) { >+ const char *name; >+ >+ idx = nla_get_u32(tb[ETHTOOL_A_BIT_INDEX]); >+ if (idx >= nbits) { >+ NL_SET_ERR_MSG_ATTR(extack, >+ tb[ETHTOOL_A_BIT_INDEX], >+ "bit index too high"); >+ return -EOPNOTSUPP; >+ } >+ name = names ? names[idx] : NULL; >+ if (tb[ETHTOOL_A_BIT_NAME] && name && >+ strncmp(nla_data(tb[ETHTOOL_A_BIT_NAME]), name, >+ nla_len(tb[ETHTOOL_A_BIT_NAME]))) { >+ NL_SET_ERR_MSG_ATTR(extack, bit_attr, >+ "bit index and name mismatch"); >+ return -EINVAL; >+ } >+ } else if (tb[ETHTOOL_A_BIT_NAME]) { >+ idx = ethnl_name_to_idx(names, nbits, >+ nla_data(tb[ETHTOOL_A_BIT_NAME]), >+ nla_len(tb[ETHTOOL_A_BIT_NAME])); It's a string? Policy validation should take care if it is correctly terminated by '\0'. Then you don't need to pass len down. Anyone who is interested in length can use strlen(). >+ if (idx >= nbits) { >+ NL_SET_ERR_MSG_ATTR(extack, >+ tb[ETHTOOL_A_BIT_NAME], >+ "bit name not found"); >+ return -EOPNOTSUPP; >+ } >+ } else { >+ NL_SET_ERR_MSG_ATTR(extack, bit_attr, >+ "neither bit index nor name specified"); >+ return -EINVAL; >+ } >+ >+ *index = idx; >+ *val = is_list || tb[ETHTOOL_A_BIT_VALUE]; >+ return 0; >+} >+ >+static int >+ethnl_update_bitset32_verbose(u32 *bitmap, unsigned int nbits, >+ const struct nlattr *attr, struct nlattr **tb, >+ ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod) >+{ >+ struct nlattr *bit_attr; >+ bool is_list; >+ int rem; >+ int ret; >+ >+ if (tb[ETHTOOL_A_BITSET_VALUE]) { >+ NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_BITSET_VALUE], >+ "value only allowed in compact bitset"); >+ return -EINVAL; >+ } >+ if (tb[ETHTOOL_A_BITSET_MASK]) { >+ NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_BITSET_MASK], >+ "mask only allowed in compact bitset"); >+ return -EINVAL; >+ } >+ is_list = (tb[ETHTOOL_A_BITSET_LIST] != NULL); just: is_list = tb[ETHTOOL_A_BITSET_LIST] is enough. >+ >+ nla_for_each_nested(bit_attr, tb[ETHTOOL_A_BITSET_BITS], rem) { >+ bool old_val, new_val; >+ unsigned int idx; >+ >+ ret = ethnl_parse_bit(&idx, &new_val, nbits, bit_attr, is_list, >+ names, extack); >+ if (ret < 0) >+ return ret; >+ old_val = bitmap[idx / 32] & ((u32)1 << (idx % 32)); >+ if (new_val != old_val) { >+ if (new_val) >+ bitmap[idx / 32] |= ((u32)1 << (idx % 32)); >+ else >+ bitmap[idx / 32] &= ~((u32)1 << (idx % 32)); >+ *mod = true; >+ } >+ } >+ >+ return 0; >+} >+ >+static int ethnl_compact_sanity_checks(unsigned int nbits, >+ const struct nlattr *nest, >+ struct nlattr **tb, >+ struct netlink_ext_ack *extack) >+{ >+ bool is_list = (tb[ETHTOOL_A_BITSET_LIST] != NULL); Same here. >+ unsigned int attr_nbits, attr_nwords; >+ const struct nlattr *test_attr; >+ >+ if (is_list && tb[ETHTOOL_A_BITSET_MASK]) { >+ NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_BITSET_MASK], >+ "mask not allowed in list bitset"); >+ return -EINVAL; >+ } >+ if (!tb[ETHTOOL_A_BITSET_SIZE]) { >+ NL_SET_ERR_MSG_ATTR(extack, nest, >+ "missing size in compact bitset"); >+ return -EINVAL; >+ } >+ if (!tb[ETHTOOL_A_BITSET_VALUE]) { >+ NL_SET_ERR_MSG_ATTR(extack, nest, >+ "missing value in compact bitset"); >+ return -EINVAL; >+ } >+ if (!is_list && !tb[ETHTOOL_A_BITSET_MASK]) { >+ NL_SET_ERR_MSG_ATTR(extack, nest, >+ "missing mask in compact nonlist bitset"); >+ return -EINVAL; >+ } >+ >+ attr_nbits = nla_get_u32(tb[ETHTOOL_A_BITSET_SIZE]); >+ attr_nwords = DIV_ROUND_UP(attr_nbits, 32); >+ if (nla_len(tb[ETHTOOL_A_BITSET_VALUE]) != attr_nwords * sizeof(u32)) { >+ NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_BITSET_VALUE], >+ "bitset value length does not match size"); >+ return -EINVAL; >+ } >+ if (tb[ETHTOOL_A_BITSET_MASK] && >+ nla_len(tb[ETHTOOL_A_BITSET_MASK]) != attr_nwords * sizeof(u32)) { >+ NL_SET_ERR_MSG_ATTR(extack, tb[ETHTOOL_A_BITSET_MASK], >+ "bitset mask length does not match size"); >+ return -EINVAL; >+ } >+ if (attr_nbits <= nbits) >+ return 0; >+ >+ test_attr = is_list ? tb[ETHTOOL_A_BITSET_VALUE] : >+ tb[ETHTOOL_A_BITSET_MASK]; >+ if (__bitmap32_not_zero(nla_data(test_attr), nbits, attr_nbits)) { >+ NL_SET_ERR_MSG_ATTR(extack, test_attr, >+ "cannot modify bits past kernel bitset size"); >+ return -EINVAL; >+ } >+ return 0; >+} >+ >+/** >+ * ethnl_update_bitset32() - Apply a bitset nest to a u32 based bitmap >+ * @bitmap: bitmap to update >+ * @nbits: size of the updated bitmap in bits >+ * @attr: nest attribute to parse and apply >+ * @names: array of bit names; may be null for compact format >+ * @extack: extack for error reporting >+ * @mod: set this to true if bitmap is modified, leave as it is if not >+ * >+ * Apply bitset netsted attribute to a bitmap. If the attribute represents >+ * a bit list, @bitmap is set to its contents; otherwise, bits in mask are >+ * set to values from value. Bitmaps in the attribute may be longer than >+ * @nbits but the message must not request modifying any bits past @nbits. >+ * >+ * Return: negative error code on failure, 0 on success >+ */ >+int ethnl_update_bitset32(u32 *bitmap, unsigned int nbits, >+ const struct nlattr *attr, ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod) >+{ >+ struct nlattr *tb[ETHTOOL_A_BITSET_MAX + 1]; >+ unsigned int change_bits; >+ bool is_list; >+ int ret; >+ >+ if (!attr) >+ return 0; >+ ret = nla_parse_nested(tb, ETHTOOL_A_BITSET_MAX, attr, bitset_policy, >+ extack); >+ if (ret < 0) >+ return ret; >+ >+ if (tb[ETHTOOL_A_BITSET_BITS]) >+ return ethnl_update_bitset32_verbose(bitmap, nbits, attr, tb, >+ names, extack, mod); >+ ret = ethnl_compact_sanity_checks(nbits, attr, tb, extack); >+ if (ret < 0) >+ return ret; >+ >+ is_list = (tb[ETHTOOL_A_BITSET_LIST] != NULL); And here. >+ change_bits = min_t(unsigned int, >+ nla_get_u32(tb[ETHTOOL_A_BITSET_SIZE]), nbits); >+ __bitmap32_update(bitmap, change_bits, >+ nla_data(tb[ETHTOOL_A_BITSET_VALUE]), >+ is_list ? NULL : nla_data(tb[ETHTOOL_A_BITSET_MASK]), >+ mod); >+ if (is_list && change_bits < nbits) >+ __bitmap32_clear(bitmap, change_bits, nbits, mod); >+ >+ return 0; >+} >+ >+/* 64-bit long endian architecture is the only case when u32 based bitmaps >+ * and unsigned long based bitmaps have different memory layout so that we >+ * cannot simply cast the latter to the former. >+ */ >+#if BITS_PER_LONG == 64 && defined(__BIG_ENDIAN) >+ >+int ethnl_bitset_size(const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact) >+{ >+ u32 small_mask32[__SMALL_BITMAP_WORDS]; >+ u32 small_val32[__SMALL_BITMAP_WORDS]; >+ u32 *mask32; >+ u32 *val32; >+ int ret; >+ >+ if (nbits > __SMALL_BITMAP_BITS) { >+ unsigned int nwords = DIV_ROUND_UP(nbits, 32); >+ >+ val32 = kmalloc_array(2 * nwords, sizeof(u32), GFP_KERNEL); >+ if (!val32) >+ return -ENOMEM; >+ mask32 = val32 + nwords; >+ } else { >+ val32 = small_val32; >+ mask32 = small_mask32; >+ } >+ >+ bitmap_to_arr32(val32, val, nbits); >+ if (mask) >+ bitmap_to_arr32(mask32, mask, nbits); >+ else >+ mask32 = NULL; >+ ret = ethnl_bitset32_size(val32, mask32, nbits, names, compact); >+ >+ if (nbits > __SMALL_BITMAP_BITS) >+ kfree(val32); >+ >+ return ret; >+} >+ >+int ethnl_put_bitset(struct sk_buff *skb, int attrtype, >+ const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact) >+{ >+ u32 small_mask32[__SMALL_BITMAP_WORDS]; >+ u32 small_val32[__SMALL_BITMAP_WORDS]; >+ u32 *mask32; >+ u32 *val32; >+ int ret; >+ >+ if (nbits > __SMALL_BITMAP_BITS) { >+ unsigned int nwords = DIV_ROUND_UP(nbits, 32); >+ >+ val32 = kmalloc_array(2 * nwords, sizeof(u32), GFP_KERNEL); >+ if (!val32) >+ return -ENOMEM; >+ mask32 = val32 + nwords; >+ } else { >+ val32 = small_val32; >+ mask32 = small_mask32; >+ } >+ >+ bitmap_to_arr32(val32, val, nbits); >+ if (mask) >+ bitmap_to_arr32(mask32, mask, nbits); >+ else >+ mask32 = NULL; >+ ret = ethnl_put_bitset32(skb, attrtype, val32, mask32, nbits, names, >+ compact); >+ >+ if (nbits > __SMALL_BITMAP_BITS) >+ kfree(val32); >+ >+ return ret; >+} >+ >+int ethnl_update_bitset(unsigned long *bitmap, unsigned int nbits, >+ const struct nlattr *attr, ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod) >+{ >+ u32 small_bitmap32[__SMALL_BITMAP_WORDS]; >+ u32 *bitmap32 = small_bitmap32; >+ bool u32_mod = false; >+ int ret; >+ >+ if (nbits > __SMALL_BITMAP_BITS) { >+ unsigned int dst_words = DIV_ROUND_UP(nbits, 32); >+ >+ bitmap32 = kmalloc_array(dst_words, sizeof(u32), GFP_KERNEL); >+ if (!bitmap32) >+ return -ENOMEM; >+ } >+ >+ bitmap_to_arr32(bitmap32, bitmap, nbits); >+ ret = ethnl_update_bitset32(bitmap32, nbits, attr, names, extack, >+ &u32_mod); >+ if (ulong_mod) { >+ bitmap_from_arr32(bitmap, bitmap32, nbits); >+ *mod = true; >+ } >+ >+ if (size > __SMALL_BITMAP_BITS) >+ kfree(bitmask32); >+ >+ return ret; >+} >+ >+#else >+ >+int ethnl_bitset_size(const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact) >+{ >+ return ethnl_bitset32_size((const u32 *)val, (const u32 *)mask, nbits, >+ names, compact); >+} >+ >+int ethnl_put_bitset(struct sk_buff *skb, int attrtype, >+ const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact) >+{ >+ return ethnl_put_bitset32(skb, attrtype, (const u32 *)val, >+ (const u32 *)mask, nbits, names, compact); >+} >+ >+int ethnl_update_bitset(unsigned long *bitmap, unsigned int nbits, >+ const struct nlattr *attr, ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod) >+{ >+ return ethnl_update_bitset32((u32 *)bitmap, nbits, attr, names, extack, >+ mod); >+} >+ >+#endif /* BITS_PER_LONG == 64 && defined(__BIG_ENDIAN) */ >diff --git a/net/ethtool/bitset.h b/net/ethtool/bitset.h >new file mode 100644 >index 000000000000..cd3d681b4524 >--- /dev/null >+++ b/net/ethtool/bitset.h >@@ -0,0 +1,28 @@ >+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ >+ >+#ifndef _NET_ETHTOOL_BITSET_H >+#define _NET_ETHTOOL_BITSET_H >+ >+typedef const char (*const ethnl_string_array_t)[ETH_GSTRING_LEN]; >+ >+int ethnl_bitset_is_compact(const struct nlattr *bitset, bool *compact); >+int ethnl_bitset_size(const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact); >+int ethnl_bitset32_size(const u32 *val, const u32 *mask, unsigned int nbits, >+ ethnl_string_array_t names, bool compact); >+int ethnl_put_bitset(struct sk_buff *skb, int attrtype, >+ const unsigned long *val, const unsigned long *mask, >+ unsigned int nbits, ethnl_string_array_t names, >+ bool compact); >+int ethnl_put_bitset32(struct sk_buff *skb, int attrtype, const u32 *val, >+ const u32 *mask, unsigned int nbits, >+ ethnl_string_array_t names, bool compact); >+int ethnl_update_bitset(unsigned long *bitmap, unsigned int nbits, >+ const struct nlattr *attr, ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod); >+int ethnl_update_bitset32(u32 *bitmap, unsigned int nbits, >+ const struct nlattr *attr, ethnl_string_array_t names, >+ struct netlink_ext_ack *extack, bool *mod); Hmm, I wonder why user needs to work with the 32 variants.. >+ >+#endif /* _NET_ETHTOOL_BITSET_H */ >diff --git a/net/ethtool/netlink.h b/net/ethtool/netlink.h >index f7c0368a9fa0..4c0b5ca439f8 100644 >--- a/net/ethtool/netlink.h >+++ b/net/ethtool/netlink.h >@@ -20,6 +20,15 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd, > u16 hdr_attrtype, struct genl_info *info, > void **ehdrp); > >+#if BITS_PER_LONG == 64 && defined(__BIG_ENDIAN) >+void ethnl_bitmap_to_u32(unsigned long *bitmap, unsigned int nwords); >+#else >+static inline void ethnl_bitmap_to_u32(unsigned long *bitmap, >+ unsigned int nwords) >+{ >+} >+#endif >+ > /** > * ethnl_strz_size() - calculate attribute length for fixed size string > * @s: ETH_GSTRING_LEN sized string (may not be null terminated) >-- >2.23.0 >