| Message ID | 20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-v2-1-1cf8958f6e8e@kernel.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [ipsec-next,v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild() | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Ping? This is still seen on next-20240909. On Thu, Aug 29, 2024 at 11:14:54AM -0700, Nathan Chancellor wrote: > Clang warns (or errors with CONFIG_WERROR): > > net/xfrm/xfrm_policy.c:1286:8: error: variable 'dir' is uninitialized when used here [-Werror,-Wuninitialized] > 1286 | if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) { > | ^~~ > net/xfrm/xfrm_policy.c:1257:9: note: initialize the variable 'dir' to silence this warning > 1257 | int dir; > | ^ > | = 0 > 1 error generated. > > A recent refactoring removed some assignments to dir because > xfrm_policy_is_dead_or_sk() has a dir assignment in it. However, dir is > used elsewhere in xfrm_hash_rebuild(), including within loops where it > needs to be reloaded for each policy. Restore the assignments before the > first use of dir to fix the warning and ensure dir is properly > initialized throughout the function. > > Fixes: 08c2182cf0b4 ("xfrm: policy: use recently added helper in more places") > Acked-by: Florian Westphal <fw@strlen.de> > Signed-off-by: Nathan Chancellor <nathan@kernel.org> > --- > Changes in v2: > - Restore another dir assignment in > list_for_each_entry_reverse(policy, ... > loop, necessitating a value reload to avoid a stale value (thanks to > Florian for the review). > - Reword commit message slightly based on above change. > - Pick up Florian's ack. > - Add 'ipsec-next' subject prefix. > - Link to v1: https://lore.kernel.org/r/20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-v1-1-a200865497b1@kernel.org > --- > net/xfrm/xfrm_policy.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c > index 6336baa8a93c..63890c0628c4 100644 > --- a/net/xfrm/xfrm_policy.c > +++ b/net/xfrm/xfrm_policy.c > @@ -1283,6 +1283,7 @@ static void xfrm_hash_rebuild(struct work_struct *work) > if (xfrm_policy_is_dead_or_sk(policy)) > continue; > > + dir = xfrm_policy_id2dir(policy->index); > if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) { > if (policy->family == AF_INET) { > dbits = rbits4; > @@ -1337,6 +1338,7 @@ static void xfrm_hash_rebuild(struct work_struct *work) > hlist_del_rcu(&policy->bydst); > > newpos = NULL; > + dir = xfrm_policy_id2dir(policy->index); > chain = policy_hash_bysel(net, &policy->selector, > policy->family, dir); > > > --- > base-commit: 17163f23678c7599e40758d7b96f68e3f3f2ea15 > change-id: 20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-749ca2264581 > > Best regards, > -- > Nathan Chancellor <nathan@kernel.org> >
On Mon, Sep 09, 2024 at 08:09:34AM -0700, Nathan Chancellor wrote:
> Ping? This is still seen on next-20240909.
Sorry, there was some delay due to travelling on my side.
This is now applied to ipsec-next, thanks Nathan!
On Tue, Sep 10, 2024 at 08:42:39AM +0200, Steffen Klassert wrote: > On Mon, Sep 09, 2024 at 08:09:34AM -0700, Nathan Chancellor wrote: > > Ping? This is still seen on next-20240909. > > Sorry, there was some delay due to travelling on my side. No worries, just wanted to make sure it did not get lost in the rush up to the merge window :) > This is now applied to ipsec-next, thanks Nathan! Thanks a lot! Cheers, Nathan
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 6336baa8a93c..63890c0628c4 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -1283,6 +1283,7 @@ static void xfrm_hash_rebuild(struct work_struct *work) if (xfrm_policy_is_dead_or_sk(policy)) continue; + dir = xfrm_policy_id2dir(policy->index); if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) { if (policy->family == AF_INET) { dbits = rbits4; @@ -1337,6 +1338,7 @@ static void xfrm_hash_rebuild(struct work_struct *work) hlist_del_rcu(&policy->bydst); newpos = NULL; + dir = xfrm_policy_id2dir(policy->index); chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
Clang warns (or errors with CONFIG_WERROR): net/xfrm/xfrm_policy.c:1286:8: error: variable 'dir' is uninitialized when used here [-Werror,-Wuninitialized] 1286 | if ((dir & XFRM_POLICY_MASK) == XFRM_POLICY_OUT) { | ^~~ net/xfrm/xfrm_policy.c:1257:9: note: initialize the variable 'dir' to silence this warning 1257 | int dir; | ^ | = 0 1 error generated. A recent refactoring removed some assignments to dir because xfrm_policy_is_dead_or_sk() has a dir assignment in it. However, dir is used elsewhere in xfrm_hash_rebuild(), including within loops where it needs to be reloaded for each policy. Restore the assignments before the first use of dir to fix the warning and ensure dir is properly initialized throughout the function. Fixes: 08c2182cf0b4 ("xfrm: policy: use recently added helper in more places") Acked-by: Florian Westphal <fw@strlen.de> Signed-off-by: Nathan Chancellor <nathan@kernel.org> --- Changes in v2: - Restore another dir assignment in list_for_each_entry_reverse(policy, ... loop, necessitating a value reload to avoid a stale value (thanks to Florian for the review). - Reword commit message slightly based on above change. - Pick up Florian's ack. - Add 'ipsec-next' subject prefix. - Link to v1: https://lore.kernel.org/r/20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-v1-1-a200865497b1@kernel.org --- net/xfrm/xfrm_policy.c | 2 ++ 1 file changed, 2 insertions(+) --- base-commit: 17163f23678c7599e40758d7b96f68e3f3f2ea15 change-id: 20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-749ca2264581 Best regards,