| Message ID | 20210312200834.370667-1-alobakin@pm.me (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a25f822285420486f5da434efc8d940d42a83bce |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] flow_dissector: fix byteorder of dissected ICMP ID | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: lariel@mellanox.com kafai@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 1 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 8 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 1 |
| netdev/header_inline | success | Link |
On Fri, Mar 12, 2021 at 09:08 PM CET, Alexander Lobakin wrote: > flow_dissector_key_icmp::id is of type u16 (CPU byteorder), > ICMP header has its ID field in network byteorder obviously. > Sparse says: > > net/core/flow_dissector.c:178:43: warning: restricted __be16 degrades to integer > > Convert ID value to CPU byteorder when storing it into > flow_dissector_key_icmp. > > Fixes: 5dec597e5cd0 ("flow_dissector: extract more ICMP information") > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > --- > net/core/flow_dissector.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > index 2ef2224b3bff..a96a4f5de0ce 100644 > --- a/net/core/flow_dissector.c > +++ b/net/core/flow_dissector.c > @@ -176,7 +176,7 @@ void skb_flow_get_icmp_tci(const struct sk_buff *skb, > * avoid confusion with packets without such field > */ > if (icmp_has_id(ih->type)) > - key_icmp->id = ih->un.echo.id ? : 1; > + key_icmp->id = ih->un.echo.id ? ntohs(ih->un.echo.id) : 1; > else > key_icmp->id = 0; > } Smells like a breaking change for existing consumers of this value. How about we change the type of flow_dissector_key_icmp{}.id to __be16 instead to make sparse happy?
On Sun, Mar 14, 2021 at 09:21:40PM +0100, Jakub Sitnicki wrote: > On Fri, Mar 12, 2021 at 09:08 PM CET, Alexander Lobakin wrote: > > flow_dissector_key_icmp::id is of type u16 (CPU byteorder), > > ICMP header has its ID field in network byteorder obviously. > > Sparse says: > > > > net/core/flow_dissector.c:178:43: warning: restricted __be16 degrades to integer > > > > Convert ID value to CPU byteorder when storing it into > > flow_dissector_key_icmp. > > > > Fixes: 5dec597e5cd0 ("flow_dissector: extract more ICMP information") > > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > > --- > > net/core/flow_dissector.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c > > index 2ef2224b3bff..a96a4f5de0ce 100644 > > --- a/net/core/flow_dissector.c > > +++ b/net/core/flow_dissector.c > > @@ -176,7 +176,7 @@ void skb_flow_get_icmp_tci(const struct sk_buff *skb, > > * avoid confusion with packets without such field > > */ > > if (icmp_has_id(ih->type)) > > - key_icmp->id = ih->un.echo.id ? : 1; > > + key_icmp->id = ih->un.echo.id ? ntohs(ih->un.echo.id) : 1; > > else > > key_icmp->id = 0; > > } > > Smells like a breaking change for existing consumers of this value. > > How about we change the type of flow_dissector_key_icmp{}.id to __be16 > instead to make sparse happy? The struct flow_dissector_key_icmp::id only appears to be used in bond_xmit_hash, and there, the exact value doesn't seem to matter. This appears to be a real bug and not just to appease sparse: ih->un.echo.id has one endianness and "1" has another. Both cannot be correct.
From: Vladimir Oltean <vladimir.oltean@nxp.com> Date: Sun, 14 Mar 2021 20:44:49 +0000 > On Sun, Mar 14, 2021 at 09:21:40PM +0100, Jakub Sitnicki wrote: >> On Fri, Mar 12, 2021 at 09:08 PM CET, Alexander Lobakin wrote: >> >> Smells like a breaking change for existing consumers of this value. >> >> How about we change the type of flow_dissector_key_icmp{}.id to __be16 >> instead to make sparse happy? > > The struct flow_dissector_key_icmp::id only appears to be used in > bond_xmit_hash, and there, the exact value doesn't seem to matter. > > This appears to be a real bug and not just to appease sparse: > ih->un.echo.id has one endianness and "1" has another. Both cannot > be correct. Agreed, so I will apply this, thanks.
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 12 Mar 2021 20:08:57 +0000 you wrote: > flow_dissector_key_icmp::id is of type u16 (CPU byteorder), > ICMP header has its ID field in network byteorder obviously. > Sparse says: > > net/core/flow_dissector.c:178:43: warning: restricted __be16 degrades to integer > > Convert ID value to CPU byteorder when storing it into > flow_dissector_key_icmp. > > [...] Here is the summary with links: - [net] flow_dissector: fix byteorder of dissected ICMP ID https://git.kernel.org/netdev/net/c/a25f82228542 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 2ef2224b3bff..a96a4f5de0ce 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -176,7 +176,7 @@ void skb_flow_get_icmp_tci(const struct sk_buff *skb, * avoid confusion with packets without such field */ if (icmp_has_id(ih->type)) - key_icmp->id = ih->un.echo.id ? : 1; + key_icmp->id = ih->un.echo.id ? ntohs(ih->un.echo.id) : 1; else key_icmp->id = 0; }
flow_dissector_key_icmp::id is of type u16 (CPU byteorder), ICMP header has its ID field in network byteorder obviously. Sparse says: net/core/flow_dissector.c:178:43: warning: restricted __be16 degrades to integer Convert ID value to CPU byteorder when storing it into flow_dissector_key_icmp. Fixes: 5dec597e5cd0 ("flow_dissector: extract more ICMP information") Signed-off-by: Alexander Lobakin <alobakin@pm.me> --- net/core/flow_dissector.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.30.2