MIME-Version: 1.0
In-Reply-To: <20171228150019.27953-1-ynorov@caviumnetworks.com>
References: <20171228150019.27953-1-ynorov@caviumnetworks.com>
From: Andy Shevchenko <andy.shevchenko@gmail.com>
Date: Sun, 31 Dec 2017 14:34:42 +0200
Message-ID: <CAHp75VfQzuq3Npo_M_Aggykxi_iJKi7dQQNY8d5t-hbd6Tr+MA@mail.gmail.com>
Subject: Re: [PATCH 1/2] bitmap: new bitmap_copy_safe and bitmap_{from,to}_arr32
To: Yury Norov <ynorov@caviumnetworks.com>
Cc: Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Ben Hutchings <ben@decadent.org.uk>,
        David Decotigny <decot@googlers.com>,
        "David S . Miller" <davem@davemloft.net>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Matthew Wilcox <mawilcox@microsoft.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4854
Lines: 122

On Thu, Dec 28, 2017 at 5:00 PM, Yury Norov <ynorov@caviumnetworks.com> wrote:
> This patchset replaces bitmap_{to,from}_u32array with more simple
> and standard looking copy-like functions.
>
> bitmap_from_u32array() takes 4 arguments (bitmap_to_u32array is similar):
>  - unsigned long *bitmap, which is destination;
>  - unsigned int nbits, the length of destination bitmap, in bits;
>  - const u32 *buf, the source; and
>  - unsigned int nwords, the length of source buffer in ints.
>
> In description to the function it is detailed like:
> * copy min(nbits, 32*nwords) bits from @buf to @bitmap, remaining
> * bits between nword and nbits in @bitmap (if any) are cleared.
>
> Having two size arguments looks unneeded and potentially dangerous.

For the first argument, it depends what logic we would like to put behind.
Imagine the case (and since these functions are targetting some wider
cases) when you have not aligned bitmap (nbits % BITS_PER_LONG != 0).

So, there are 2 cases, nwords > nbits / BITS_PER_U32, or nbits /
BITS_PER_U32 > nwords.

We have at least two options for the first one:
1) cut it to fit and return some code (or updated nbits, or ...) to
tell this to the caller;
2) return an error and do nothing.

For the second case one:
1) merge bitmaps;
2) fill with 0 or 1 (another parameter?) the rest of bitmap.

> It is unneeded because normally user of copy-like function should
> take care of the size of destination and make it big enough to fit
> source data.
>
> And it is dangerous because function may hide possible error if user
> doesn't provide big enough bitmap, and data becomes silently dropped.

We might return -E2BIG, for example.

> That's why all copy-like functions have 1 argument for size of copying
> data, and I don't see any reason to make bitmap_from_u32array()
> different.
>
> One exception that comes in mind is strncpy() which also provides size
> of destination in arguments, but it's strongly argued by the possibility
> of taking broken strings in source. This is not the case of
> bitmap_{from,to}_u32array().
>
> There is no many real users of bitmap_{from,to}_u32array(), and they all
> very clearly provide size of destination matched with the size of
> source, so additional functionality is not used in fact. Like this:
> bitmap_from_u32array(to->link_modes.supported,
>                 __ETHTOOL_LINK_MODE_MASK_NBITS,
>                 link_usettings.link_modes.supported,
>                 __ETHTOOL_LINK_MODE_MASK_NU32);
> Where:
> #define __ETHTOOL_LINK_MODE_MASK_NU32 \
>         DIV_ROUND_UP(__ETHTOOL_LINK_MODE_MASK_NBITS, 32)

Consider more generic use of them.

For example, we have a big enough bitmap, but would like to copy only
few u32 items to it. Another case, we have quite big u32 array, but
would like to copy only some of them.
It is first what came to my mind, there are might be much more
interesting corner cases.

Will it survive?

> In this patch, bitmap_copy_safe and bitmap_{from,to}_arr32 are introduced.
>
> 'Safe' in bitmap_copy_safe() stands for clearing unused bits in bitmap
> beyond last bit till the end of last word. It is useful for hardening
> API when bitmap is assumed to be exposed to userspace.

Why not setting them? How do you know it's used in positive manner and
not negative (inverted)?

Another thing, it shouldn't be different by intuition how it behaves
against bitmap, either both magle it, or none. Or just choose another
(better) name. _safe sounds a bit confusing to me.

> bitmap_{from,to}_arr32 functions are replacements for
> bitmap_{from,to}_u32array. They don't take unneeded nwords argument, and
> so simpler in implementation and understanding.

Again, depends what logic we would like to have.
I can imagine the case when you have nbits defined as macro/constant
and wouldn't like to change over the code. In your case you
semantically substitute nbits of bitmap, by nbits of amount of data to
copy.

For me slightly cleaner to have full size of the destination buffer at
input rather than arbitrary number which might not prevent crashes in
some cases.

Consider snprintf() as example.

> This patch suggests optimization for 32-bit systems - aliasing
> bitmap_{from,to}_arr32 to bitmap_copy_safe.

So, you preventing *merge* by this. Not sure if it's desired.

> Other possible optimization is aliasing 64-bit LE bitmap_{from,to}_arr32 to
> more generic function(s). But I didn't end up with the function that would
> be helpful by itself, and can be used to alias 64-bit LE
> bitmap_{from,to}_arr32, like bitmap_copy_safe() does. So I preferred to
> leave things as is.
>
> The following patch switches kernel to new API and introduces test for it.
>
> Discussion is here.
> https://lkml.org/lkml/2017/11/15/592

Unfortunately didn't find any _discussion_ there...
Wasn't it elsewhere?

-- 
With Best Regards,
Andy Shevchenko