[PATCH] net: Fix special case of empty range in find_next_netdev_feature()

[PATCH] net: Fix special case of empty range in find_next_netdev_feature()

[Joachim Foerster]

Avoids running into an infinite loop when the lowest feature bit is
asserted.

In case of the "start" argument of find_next_netdev_feature() being 0, the
result will be the index of the highest asserted bit in its argument
"feature".  Given that for_each_netdev_feature() uses the return value of
find_next_netdev_feature() - which can of course be 0 (=> bit index 0) - as
the next "start" value, find_next_netdev_feature() has to deal with that,
in order to make sure that the loop of for_each_netdev_feature() ends when
having iterated over all asserted bits.

Fixes: 85db6352fc8a ("net: Fix features skip in for_each_netdev_feature()")
Cc: stable@vger.kernel.org
Signed-off-by: Joachim Foerster <joachim.foerster@missinglinkelectronics.com>
---
 include/linux/netdev_features.h | 6 ++++++
 1 file changed, 6 insertions(+)

Of course one could also argue, that this should be fixed in
for_each_netdev_feature() itself. However that could complicate this macro.

On the other hand, I don't know whether there is a possibility to also
cover the special case as part of the masking and shifting in
find_next_netdev_feature().

Since the past commit 85db6352fc8a has introduced in 5.18, this fix here
could be queued for -stable >= 5.18. And should probably be queued for >=
6.1?

diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
index 7c2d77d75a88..30f5364c2e85 100644
--- a/include/linux/netdev_features.h
+++ b/include/linux/netdev_features.h
@@ -173,6 +173,12 @@ enum {
  */
 static inline int find_next_netdev_feature(u64 feature, unsigned long start)
 {
+	/* catch special case of start == 0, which indicates "empty range";
+	 * caller for_each_netdev_feature() depends on this
+	 */
+	if (unlikely(!start))
+		return -1;
+
 	/* like BITMAP_LAST_WORD_MASK() for u64
 	 * this sets the most significant 64 - start to 0.
 	 */
-- 
2.17.1

Re: [PATCH] net: Fix special case of empty range in find_next_netdev_feature()

[Jakub Kicinski]

On Fri, 23 Jun 2023 16:26:16 +0200 Joachim Foerster wrote:
> Fixes: 85db6352fc8a ("net: Fix features skip in for_each_netdev_feature()")
> Cc: stable@vger.kernel.org

Nothing passes @feature with bit 0 set upstream, tho, right?
Fix looks fine, but it doesn't need the fixes tag and CC stable,
since its theoretical/forward looking.

Please repost explaining how we can hit this problem upstream
or with the Fixes/CC stable replaced by a sentence stating that
the problem can't currently be triggered.
-- 
pw-bot: cr

Re: [PATCH] net: Fix special case of empty range in find_next_netdev_feature()

[Joachim Förster]

On 6/26/23 23:17, Jakub Kicinski wrote:
> On Fri, 23 Jun 2023 16:26:16 +0200 Joachim Foerster wrote:
>> Fixes: 85db6352fc8a ("net: Fix features skip in for_each_netdev_feature()")
>> Cc: stable@vger.kernel.org
> Nothing passes @feature with bit 0 set upstream, tho, right?
> Fix looks fine, but it doesn't need the fixes tag and CC stable,
> since its theoretical/forward looking.
We are triggering this issue by using the inline function 
for_each_netdev_feature() from the kernel header files in a custom 
module running on a stable kernel involving NETIF_F_SG, which happens to 
be bit 0. So my argument is that the function is part of the public API. 
Or is this actually not supposed to be treated like it is public API? 
Does this statement change the assessment in terms of tagging with CC 
stable?

Regarding the Fixes tag, I think, I made a mistake, since the previous 
commit 3b89ea9c5902 ("net: Fix for_each_netdev_feature on Big endian") 
on find_next_netdev_feature() already causes the issue by not 
considering the special case of bit 0. So I will repost with fixes tag 
updated ...

>
> Please repost explaining how we can hit this problem upstream
> or with the Fixes/CC stable replaced by a sentence stating that
> the problem can't currently be triggered.


-- 
Joachim Förster

Missing Link Electronics
http://www.missinglinkelectronics.com
Office DE: +49 (731) 141-149-0
Office US: +1  (408) 457-0700

Re: [PATCH] net: Fix special case of empty range in find_next_netdev_feature()

[Jakub Kicinski]

On Wed, 5 Jul 2023 15:40:29 +0200 Joachim Förster wrote:
> On 6/26/23 23:17, Jakub Kicinski wrote:
> > On Fri, 23 Jun 2023 16:26:16 +0200 Joachim Foerster wrote:  
> >> Fixes: 85db6352fc8a ("net: Fix features skip in for_each_netdev_feature()")
> >> Cc: stable@vger.kernel.org  
> > Nothing passes @feature with bit 0 set upstream, tho, right?
> > Fix looks fine, but it doesn't need the fixes tag and CC stable,
> > since its theoretical/forward looking.  
> We are triggering this issue by using the inline function 
> for_each_netdev_feature() from the kernel header files in a custom 
> module running on a stable kernel involving NETIF_F_SG, which happens to 
> be bit 0. So my argument is that the function is part of the public API. 
> Or is this actually not supposed to be treated like it is public API? 
> Does this statement change the assessment in terms of tagging with CC 
> stable?

I believe so, if an upstream user can't hit the problem its not
a bug for upstream. I'm not familiar with the concept of public 
API, but I'm afraid it may be a bit of a pandora's box. We have
Documentation/process/stable-api-nonsense.rst, tho, I'm not sure 
it applies to this case.

> Regarding the Fixes tag, I think, I made a mistake, since the previous 
> commit 3b89ea9c5902 ("net: Fix for_each_netdev_feature on Big endian") 
> on find_next_netdev_feature() already causes the issue by not 
> considering the special case of bit 0. So I will repost with fixes tag 
> updated ...

For networking stable == fixes more or less, so if it's not a bug
fix it should not have a Fixes tag either. But we're not maintaining
stable ourselves, we primarily care about describing the situation
and tagging appropriately in the commit message. You can still try
to convince Greg KH to pull it into stable afterwards. Who knows, 
maybe Sasha's AI will even suck it in automatically..

> > Please repost explaining how we can hit this problem upstream
> > or with the Fixes/CC stable replaced by a sentence stating that
> > the problem can't currently be triggered.