[PATCH ipsec-next v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild()

[PATCH ipsec-next v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild()

[Nathan Chancellor]

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>

Re: [PATCH ipsec-next v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild()

[Nathan Chancellor]

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>
> 

Re: [PATCH ipsec-next v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild()

[Steffen Klassert]

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!

Re: [PATCH ipsec-next v2] xfrm: policy: Restore dir assignments in xfrm_hash_rebuild()

[Nathan Chancellor]

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