[PATCH] xfrm: policy: Restore dir assignment in xfrm_hash_rebuild()

[PATCH] xfrm: policy: Restore dir assignment 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 the assignment of dir because
xfrm_policy_is_dead_or_sk() has a dir assignment in it. However, dir is
used elsewhere in xfrm_hash_rebuild(), so restore the assignment to fix
the warning and ensure dir is initialized throughout the function.

Fixes: 08c2182cf0b4 ("xfrm: policy: use recently added helper in more places")
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 net/xfrm/xfrm_policy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 6336baa8a93c..02eb4bd0fde6 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;

---
base-commit: 17163f23678c7599e40758d7b96f68e3f3f2ea15
change-id: 20240829-xfrm-restore-dir-assign-xfrm_hash_rebuild-749ca2264581

Best regards,
-- 
Nathan Chancellor <nathan@kernel.org>

Re: [PATCH] xfrm: policy: Restore dir assignment in xfrm_hash_rebuild()

[Florian Westphal]

Nathan Chancellor <nathan@kernel.org> 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.

Ugh, my bad.

Acked-by: Florian Westphal <fw@strlen.de>

Re: [PATCH] xfrm: policy: Restore dir assignment in xfrm_hash_rebuild()

[Florian Westphal]

Florian Westphal <fw@strlen.de> wrote:
> Nathan Chancellor <nathan@kernel.org> 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.
> 
> Ugh, my bad.
> 
> Acked-by: Florian Westphal <fw@strlen.de>

Actually, this fix is incomplete, the assignment needs to be
restored in the second loop as well:

1340                 chain = policy_hash_bysel(net, &policy->selector,
1341                                           policy->family, dir);
							       ~~~

Nathan, Steffen, I'll leave it up to you to either do a v2 or a revert.

Re: [PATCH] xfrm: policy: Restore dir assignment in xfrm_hash_rebuild()

[Nathan Chancellor]

On Thu, Aug 29, 2024 at 08:02:05PM +0200, Florian Westphal wrote:
> Florian Westphal <fw@strlen.de> wrote:
> > Nathan Chancellor <nathan@kernel.org> 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.
> > 
> > Ugh, my bad.
> > 
> > Acked-by: Florian Westphal <fw@strlen.de>
> 
> Actually, this fix is incomplete, the assignment needs to be
> restored in the second loop as well:
> 
> 1340                 chain = policy_hash_bysel(net, &policy->selector,
> 1341                                           policy->family, dir);
> 							       ~~~
> 
> Nathan, Steffen, I'll leave it up to you to either do a v2 or a revert.

Ah, good catch. I'll send a v2 shortly with this fixed and your ack
picked up.

Cheers,
Nathan