Re: [PATCH ipsec-next 1/2] xfrm: introduce forwarding of ICMP Error messages

[Dan Carpenter <dan.carpenter@linaro.org>]

On Wed, Jan 31, 2024 at 08:38:51PM +0100, Antony Antony wrote:
> HI Dan,
> 
> Thanks for reporting the warning.
> 
> On Tue, Jan 30, 2024 at 01:36:28PM +0300, Dan Carpenter wrote:
> > 
> > Hello Antony Antony,
> > 
> > The patch 63b21caba17e: "xfrm: introduce forwarding of ICMP Error
> > messages" from Jan 19, 2024 (linux-next), leads to the following
> > Smatch static checker warning:
> > 
> > 	net/xfrm/xfrm_policy.c:3708 __xfrm_policy_check()
> > 	error: testing array offset 'dir' after use.
> 
> > 
> > net/xfrm/xfrm_policy.c
> >   3689  
> >   3690          pol = NULL;
> >   3691          sk = sk_to_full_sk(sk);
> >   3692          if (sk && sk->sk_policy[dir]) {
> >                             ^^^^^^^^^^^^^^^^
> > If dir is XFRM_POLICY_FWD (2) then it is one element beyond the end of
> > the ->sk_policy[] array.
> 
> Yes, that's correct. However, for this patch, it's necessary that sk != NULL 
> at the same time. As far as I know, there isn't any code that would call dir 
> = XFRM_POLICY_FWD with sk != NULL. What am I missing? Did Smatch give any 
> hints for such a code path?
> 

I wondered if that might be the case.  The truth is that this sort of
dependency is too compicated for any static analysis tools that
currently exist.  Smatch tries to track the relationship between
"dir" and "sk" as they are passed in, but it will look the relationship
information when we re-assign sk.  "sk = sk_to_full_sk(sk);".

So what we do in this case, is we just ignore the warning and if anyone
has questions about it they will look up this conversation on
lore.kernel.org to find the explanation.

No need to worry about trying to silence the checker...

regards,
dan carpenter