[PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Everest K.C.]

Error handling is missing when call to nla_put_u32() fails.
Handle the error when the call to nla_put_u32() returns an error.

The error was reported by Coverity Scan.
Report:
CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
to err here, but that stored value is overwritten before it can be used

Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
---
 net/xfrm/xfrm_user.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index f0ee0c7a59dd..a784598cc7cf 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2607,9 +2607,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
 	err = xfrm_if_id_put(skb, x->if_id);
 	if (err)
 		goto out_cancel;
-	if (x->pcpu_num != UINT_MAX)
+	if (x->pcpu_num != UINT_MAX) {
 		err = nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num);
-
+		if (err)
+			goto out_cancel;
+	}
+
 	if (x->dir) {
 		err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
 		if (err)
-- 
2.43.0

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Simon Horman]

On Tue, Nov 12, 2024 at 04:36:06PM -0700, Everest K.C. wrote:
> Error handling is missing when call to nla_put_u32() fails.
> Handle the error when the call to nla_put_u32() returns an error.
> 
> The error was reported by Coverity Scan.
> Report:
> CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> to err here, but that stored value is overwritten before it can be used
> 
> Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>

Reviewed-by: Simon Horman <horms@kernel.org>

For future reference, I think the appropriate target for this tree
is ipsec-next rather than next.

	Subject: [PATCH ipsec-next] xfrm: ...

...

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Dan Carpenter]

On Wed, Nov 13, 2024 at 10:59:39AM +0000, Simon Horman wrote:
> On Tue, Nov 12, 2024 at 04:36:06PM -0700, Everest K.C. wrote:
> > Error handling is missing when call to nla_put_u32() fails.
> > Handle the error when the call to nla_put_u32() returns an error.
> > 
> > The error was reported by Coverity Scan.
> > Report:
> > CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> > returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> > to err here, but that stored value is overwritten before it can be used
> > 
> > Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> For future reference, I think the appropriate target for this tree
> is ipsec-next rather than next.
> 
> 	Subject: [PATCH ipsec-next] xfrm: ...

All these trees are a pain in the butt to track.  It's fine for people who only
work in one tree but for people doing static checker stuff, then we have to
deal with all 388 trees in linux-next.

I've changed my scripts to add [next] to my patches if Linus hasn't merged the
commit from the Fixes tag.  I still add net and net-next by hand but I'm going
to just automate that as well because doing it by hand has been failure prone.

But then if we try to add all the ipsec or whatever trees, it just becomes
unworkable.  I started to write a script which would look do the --is-ancestor
check based on the Fixes tag, but it take forever to update the git trees.  I
wasn't able to figure out a way to make this work.

Also once Linus merges the commit, there is no way to tell which tree the commit
goes to so it only applies to linux-next.  For networking, I already have the
script that greps the patch for -w net and grep -vw wireless.  But I don't want
to maintain a list greps for everyone's tree.

A lot of this scripting could be built into the CI system.  The CI system is
already doing some scripting based on the subject but we could do it based on
the Fixes tag instead.  If there isn't a Fixes tag, then it should go to
net-next.

regards,
dan carpenter

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Dan Carpenter]

Here are the relevant lines from my email generator script.  The script
greps to see if netdev is in the CC list but wireless isn't.  Then it
looks up the FIXES_HASH.  You have to have to have done a recent fetch
obviously.  My process doesn't work very well for patchsets.

regards,
dan carpenter

if grep -q netdev $MAIL_FILE && ! grep -q wireless $MAIL_FILE ; then
    if [ "$FIXES_COMMIT" != "" ] ; then
        if git merge-base --is-ancestor $FIXES_COMMIT net/main ; then
            TREE=" net"
        elif git merge-base --is-ancestor $FIXES_COMMIT net-next/main ; then
            TREE=" net-next"
        fi
    else
        TREE=" net-next"
    fi
fi

if [ "$TREE" == "" ] ; then
    if [ "$FIXES_COMMIT" != "" ] ; then
        if ! git merge-base --is-ancestor $FIXES_COMMIT origin/master ; then
            TREE=" next"
        fi
    fi
fi

git format-patch HEAD^ --stdout | sed -e "s/Subject: \[PATCH\]/Subject: [PATCH${TREE}]/" >> $MAIL_FILE

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Simon Horman]

On Wed, Nov 13, 2024 at 04:10:15PM +0300, Dan Carpenter wrote:
> On Wed, Nov 13, 2024 at 10:59:39AM +0000, Simon Horman wrote:
> > On Tue, Nov 12, 2024 at 04:36:06PM -0700, Everest K.C. wrote:
> > > Error handling is missing when call to nla_put_u32() fails.
> > > Handle the error when the call to nla_put_u32() returns an error.
> > > 
> > > The error was reported by Coverity Scan.
> > > Report:
> > > CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> > > returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> > > to err here, but that stored value is overwritten before it can be used
> > > 
> > > Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> > 
> > Reviewed-by: Simon Horman <horms@kernel.org>
> > 
> > For future reference, I think the appropriate target for this tree
> > is ipsec-next rather than next.
> > 
> > 	Subject: [PATCH ipsec-next] xfrm: ...
> 
> All these trees are a pain in the butt to track.  It's fine for people who only
> work in one tree but for people doing static checker stuff, then we have to
> deal with all 388 trees in linux-next.
> 
> I've changed my scripts to add [next] to my patches if Linus hasn't merged the
> commit from the Fixes tag.  I still add net and net-next by hand but I'm going
> to just automate that as well because doing it by hand has been failure prone.
> 
> But then if we try to add all the ipsec or whatever trees, it just becomes
> unworkable.  I started to write a script which would look do the --is-ancestor
> check based on the Fixes tag, but it take forever to update the git trees.  I
> wasn't able to figure out a way to make this work.
> 
> Also once Linus merges the commit, there is no way to tell which tree the commit
> goes to so it only applies to linux-next.  For networking, I already have the
> script that greps the patch for -w net and grep -vw wireless.  But I don't want
> to maintain a list greps for everyone's tree.
> 
> A lot of this scripting could be built into the CI system.  The CI system is
> already doing some scripting based on the subject but we could do it based on
> the Fixes tag instead.  If there isn't a Fixes tag, then it should go to
> net-next.

Hi Dan,

I take your point that this is not very friendly to people sending
the occasional patch (towards Networking). And certainly there
is room to improve the CI.

FWIIW, my goto when preparing patches is something like the following.
Because at least for Networking, we do try to make MAINTAINERS reflect
reality:

./scripts/get_maintainer.pl --scm net/xfrm/xfrm_user.c

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Everest K.C.]

On Wed, Nov 13, 2024 at 3:59 AM Simon Horman <horms@kernel.org> wrote:
>
> On Tue, Nov 12, 2024 at 04:36:06PM -0700, Everest K.C. wrote:
> > Error handling is missing when call to nla_put_u32() fails.
> > Handle the error when the call to nla_put_u32() returns an error.
> >
> > The error was reported by Coverity Scan.
> > Report:
> > CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> > returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> > to err here, but that stored value is overwritten before it can be used
> >
> > Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
>
> Reviewed-by: Simon Horman <horms@kernel.org>
>
> For future reference, I think the appropriate target for this tree
> is ipsec-next rather than next.
>
>         Subject: [PATCH ipsec-next] xfrm: ...
Should I send a patch to ipsec-next ?
>
> ...
- Everest K.C.

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Steffen Klassert]

On Thu, Nov 14, 2024 at 12:27:28PM -0700, Everest K.C. wrote:
> On Wed, Nov 13, 2024 at 3:59 AM Simon Horman <horms@kernel.org> wrote:
> >
> > On Tue, Nov 12, 2024 at 04:36:06PM -0700, Everest K.C. wrote:
> > > Error handling is missing when call to nla_put_u32() fails.
> > > Handle the error when the call to nla_put_u32() returns an error.
> > >
> > > The error was reported by Coverity Scan.
> > > Report:
> > > CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> > > returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> > > to err here, but that stored value is overwritten before it can be used
> > >
> > > Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> > > Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> >
> > Reviewed-by: Simon Horman <horms@kernel.org>
> >
> > For future reference, I think the appropriate target for this tree
> > is ipsec-next rather than next.
> >
> >         Subject: [PATCH ipsec-next] xfrm: ...
> Should I send a patch to ipsec-next ?

No need to resend. This is now applied to ipsec-next,
thanks a lot!

Re: [PATCH][next] xfrm: Add error handling when nla_put_u32() returns an error

[Przemek Kitszel]

On 11/13/24 00:36, Everest K.C. wrote:
> Error handling is missing when call to nla_put_u32() fails.
> Handle the error when the call to nla_put_u32() returns an error.
> 
> The error was reported by Coverity Scan.
> Report:
> CID 1601525: (#1 of 1): Unused value (UNUSED_VALUE)
> returned_value: Assigning value from nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num)
> to err here, but that stored value is overwritten before it can be used
> 
> Fixes: 1ddf9916ac09 ("xfrm: Add support for per cpu xfrm state handling.")
> Signed-off-by: Everest K.C. <everestkc@everestkc.com.np>
> ---
>   net/xfrm/xfrm_user.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index f0ee0c7a59dd..a784598cc7cf 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2607,9 +2607,12 @@ static int build_aevent(struct sk_buff *skb, struct xfrm_state *x, const struct
>   	err = xfrm_if_id_put(skb, x->if_id);
>   	if (err)
>   		goto out_cancel;
> -	if (x->pcpu_num != UINT_MAX)
> +	if (x->pcpu_num != UINT_MAX) {
>   		err = nla_put_u32(skb, XFRMA_SA_PCPU, x->pcpu_num);
> -
> +		if (err)
> +			goto out_cancel;
> +	}
> +
>   	if (x->dir) {
>   		err = nla_put_u8(skb, XFRMA_SA_DIR, x->dir);
>   		if (err)

this is a fix indeed,
Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
--
I find nla_put*() familiy error handling very ugly for the calling code,
especially given that some of the calls are conditional

I would like to refactor it some day, to give the caller possibility to
just put all the needed fields and check the error once at the end.
Nesting complicates things a bit, but perhaps it could be also covered
in such way (didn't checked yet).