| Message ID | 20230623142616.144923-1-joachim.foerster@missinglinkelectronics.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Fix special case of empty range in find_next_netdev_feature() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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.
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.
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.
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. */
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?