Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7e98e085-a167-f69d-5fcd-358157e1741e@intel.com>
Date:   Thu, 13 Apr 2023 15:50:16 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH net-next v2] net/ipv6: silence 'passing zero to ERR_PTR()'
 warning
Content-Language: en-US
To:     Haoyi Liu <iccccc@hust.edu.cn>,
        "David S. Miller" <davem@davemloft.net>,
        David Ahern <dsahern@kernel.org>,
        Eric Dumazet <edumazet@google.com>,
        "Jakub Kicinski" <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>
CC:     <hust-os-kernel-patches@googlegroups.com>, <yalongz@hust.edu.cn>,
        <error27@gmail.com>, Dongliang Mu <dzm91@hust.edu.cn>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230413101005.7504-1-iccccc@hust.edu.cn>
From:   Jacob Keller <jacob.e.keller@intel.com>
In-Reply-To: <20230413101005.7504-1-iccccc@hust.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Ky9Za2ZYN0huM1RMaGZyTGphTzduQ2NydHhrRHlLb0Y3U3dId29sdHJoVUNy?=
 =?utf-8?B?V1dSSGYvc25GajJzNzVIcDEyS2lXb0NvNElEMWR0ZDZNZ0sveXh5OWlSVTdp?=
 =?utf-8?B?ZzZRZm5kN1MxM2k3MUJQbHNEUzBxQ0pkcXRRdk9VRzRqQm9yNmxEL3BETVRF?=
 =?utf-8?B?MVczZENCTCtUcXBNanNHL3IvWGNBcVVYM1JrVU5qUjdCQkVBd3MrVjlYeHJ0?=
 =?utf-8?B?QW55Y1phbXBaNXlvbUZBSUxhamdibko3YWhQTExCMWNxb0o4V0FwNXdScTFX?=
 =?utf-8?B?dkRmeDQzeXBXSUVjSmJ0SFE4Z0VhNm02T2pONkkrNXd0KzdleDU1Wmc4ZkE3?=
 =?utf-8?B?ZGI2bDNMNmVlKzFrbmZVcjBmbXV5eEFtU2V6ZlZrZFRtNlI0S0VVdytab2I3?=
 =?utf-8?B?ZEtvSU9Ua3BIaTFkbU5lcWh2SE94WlYya2pGbWtYb0xRc3pPM2RpZnN0bHV6?=
 =?utf-8?B?REQrTDZzdWxtUDV5WGFwRHd3d2tmdDUvODVQZUVtaWtmb2tzRFhwenRZN0o2?=
 =?utf-8?B?a0lYV1dpbytVdFI0TWxUTGFXdzJRMDNvZXJJd3FkUU5NaVY2WUFLVlpvMmJF?=
 =?utf-8?B?cjdGSDhZNUVVYzNhczJkbU5OMFU4Qm5PYml0QkZmdjlIbS90L1NFdUg2U2Fr?=
 =?utf-8?B?TEpETEFlcU9zSjEyUUdKKzFHcjJSUnpZeFRvMVBFM3VUZTEvcTNXRkd0dDQy?=
 =?utf-8?B?UXZqcnJEMVp4QUZ4VGM5cy9kcTVMNlpQeTJwN1dYZVl2amYrVGJzQlA0c0RQ?=
 =?utf-8?B?QWZVS3V1QXBxS1BodkpoWld2RXpoK3Ixc2VXTm92Q1o4SVdJMzNMMnVhZWxL?=
 =?utf-8?B?UUc1UG13dHZMV2FyOFJwYkI1UEsrOGNvMTBRZDRBU2R4ZTJFa2tiL3ZvODJq?=
 =?utf-8?B?b2FmclJXTUh2NU1pa1d2eUtRQXdENDUwL1ZoM3EvRDJ3eDUxQ2JvY3hoNndU?=
 =?utf-8?B?TWVqaGRTT0JOcWs5M2lGZlIvSFJnUzM5aHNwbVY3M09VN05xNml5TTc3OXFl?=
 =?utf-8?B?N3pjMVA1QXpVTUhmUnc3UzN6c3dEOGFGNlZ3SnN2MTk4YTRZb0xscUlGKzk4?=
 =?utf-8?B?dXNBZmFVZkpQcEs5c3M4eVNIaFcwVHExeHY4QlpOL2dkbFpYQWw4eXlSNUxk?=
 =?utf-8?B?eHFlN1RZN0JjSmwwVUNFUlRTdnpXOGtjOVdxR0NYL253TGNvM0cvc1lrcHIz?=
 =?utf-8?B?czFaRjJKMzNGMjliQ3hjNms4eGl2NzFOdDNibGNEV2ZOMFJGRUVTbjhsWVRi?=
 =?utf-8?B?RUpCNHhNN1drYzY1U20yVnJKKzB2UWtJN2dvTjFiZ1BPUW5xcUZNNzZ2cXBU?=
 =?utf-8?B?LzJnTUQ5U2xHdno1Mm1hZHVVWTltekVFVGJlV3psMndUSE5mZjBpUzljUjZG?=
 =?utf-8?B?M1NpZVdKUEFyWDVzNkgwc1ZlUEZwOFI2dkZoQ3UvcFdmNFRUa0YwZEloTnZL?=
 =?utf-8?B?M20zbGVvUFhkR3RCWllHS09Bc09PcG94UjZFNzZCbndoSDUvWjlwUE1yNFNG?=
 =?utf-8?B?bFdDcGVYR3Voc1pmRWkwK1lyejMzUUZoN29sMUg1REJ4MzUrV3Fyc29aWHY4?=
 =?utf-8?B?SEhIWWJjWlZVaXArdzdRaEJkRFVNRWJJSGZqS1JLVWJDb2p4TVl5S0FrTDBm?=
 =?utf-8?B?MnZMSXdrQ3pHeFFDcFJtNWtld0t6NVZFeXNHZTZxNU03cmNmTWZiMDV6TG9G?=
 =?utf-8?B?NENTNDFjemU5R0NJTUJTUW1pcmVwWm5rOWxnSEVSRE91RnJ3Vk5kQkp4UkZD?=
 =?utf-8?B?ZFdJQS84Qy84RFE1RjQ5MTExaGEzZFVPTkZVZm9kakt5cGUxTVFZOEhHdnpD?=
 =?utf-8?B?Sk1zQmZzV3VjWHBRdnhHYXQxTHhnQmdvQlhlcjRaQ0Faekx4RTFmL3hra1Ur?=
 =?utf-8?B?RDE4Z3VkSExJN3FNdG1GTzUxVkhSVmtkNFVpQldlUnFJdEZlZGRqZndqZEdQ?=
 =?utf-8?B?c1NyL0ZseVNHbUJVQzJBOWtVL1pMSk9LbU90UEdOVnU1c1FaeS9xbzJzRkdN?=
 =?utf-8?B?ZU1IMXFoRkxJNDFtRHVOc1IrdjNOMEJPcEN4d0lweDdNVXVyRHZyUUQyR3hY?=
 =?utf-8?B?S2ZodmliWFdxQnV6b2cyRDNYYWtVUlhmWnUyN09VZ0o0MHBzRFFCaUlRWWd1?=
 =?utf-8?B?RmE0SHdhMDFON3FZMCtnWnpFSlVra0NITjkrRzUwQVQvaURkQmhmNHhTT2lp?=
 =?utf-8?B?a2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 362dc9b6-2eb0-4e85-c773-08db3c717430
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Apr 2023 22:50:18.6394
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /Ual5gR54A0URyAVBjXFZxTX5cAoYbCU+471yZ5JNZ+VnuoF5hopeN1W4IxTAx9WyucA7vOAd8abbWnkWG/jUXHpKUMtY0mzNYmuy46YoaU=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW6PR11MB8337
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 4/13/2023 3:10 AM, Haoyi Liu wrote:
> Smatch complains that if xfrm_lookup() returns NULL then this does a
> weird thing with "err":
> 
>     net/ ipv6/ icmp.c:411 icmpv6_route_lookup()
>     warn: passing zero to ERR_PTR()
> 
> Merge conditional paths and remove unnecessary 'else'. No functional
> change.
> 
> Signed-off-by: Haoyi Liu <iccccc@hust.edu.cn>
> Reviewed-by: Dongliang Mu <dzm91@hust.edu.cn>
> ---
> v1->v2: Remove unnecessary 'else'.
> The issue is found by static analysis, and the patch is remains untested.
> ---
>  net/ipv6/icmp.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/net/ipv6/icmp.c b/net/ipv6/icmp.c
> index 1f53f2a74480..a12eef11c7ee 100644
> --- a/net/ipv6/icmp.c
> +++ b/net/ipv6/icmp.c
> @@ -393,17 +393,12 @@ static struct dst_entry *icmpv6_route_lookup(struct net *net,
>  		goto relookup_failed;
>  
>  	dst2 = xfrm_lookup(net, dst2, flowi6_to_flowi(&fl2), sk, XFRM_LOOKUP_ICMP);
> -	if (!IS_ERR(dst2)) {
> -		dst_release(dst);
> -		dst = dst2;
> -	} else {
> -		err = PTR_ERR(dst2);
> -		if (err == -EPERM) {
> -			dst_release(dst);
> -			return dst2;
> -		} else
> -			goto relookup_failed;
> -	}
> +	err = PTR_ERR_OR_ZERO(dst2);
> +	if (err && err != -EPERM)
> +		goto relookup_failed;
> +
> +	dst_release(dst);
> +	return dst2;	/* returns success or ERR_PTR(-EPERM) */
>  

This result looks much cleaner!

Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>

Thanks,
Jake

>  relookup_failed:
>  	if (dst)