Date:   Thu, 23 Feb 2023 10:59:05 +0300
From:   Dan Carpenter <error27@gmail.com>
To:     Li Qiong <liqiong@nfschina.com>
Cc:     "David S . Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org,
        Yu Zhe <yuzhe@nfschina.com>
Subject: Re: [PATCH] net/atm/mpc: Fix dereference NULL pointer in
 mpc_send_packet()
Message-ID: <Y/ccya+o7zBqMyhJ@kadam>
References: <20230223065446.24173-1-liqiong@nfschina.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230223065446.24173-1-liqiong@nfschina.com>
Precedence: bulk

On Thu, Feb 23, 2023 at 02:54:46PM +0800, Li Qiong wrote:
> The 'non_ip' statement need do 'mpc' pointer dereference,
> so return '-ENODEV' if 'mpc' is NULL.
> 
> Signed-off-by: Li Qiong <liqiong@nfschina.com>
> ---
>  net/atm/mpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/atm/mpc.c b/net/atm/mpc.c
> index 033871e718a3..1cd6610b8a12 100644
> --- a/net/atm/mpc.c
> +++ b/net/atm/mpc.c
> @@ -577,7 +577,7 @@ static netdev_tx_t mpc_send_packet(struct sk_buff *skb,
>  	mpc = find_mpc_by_lec(dev); /* this should NEVER fail */
>  	if (mpc == NULL) {
>  		pr_info("(%s) no MPC found\n", dev->name);
> -		goto non_ip;
> +		return -ENODEV;
>  	}

The comment says that find_mpc_by_lec() can't fail.  This business of
handling impossible situations is very complicated.  Should this
free the skb before returning?  Eventually static checkers will detect
that.

Generally the rule is that we don't have checks for impossible
conditions.  That would trigger a warning for certain static checkers
but it would be a false positive.  Otherwise we need to add a whole
bunch of code to silence all warnings about handling an impossible
situation correctly.

regards,
dan carpenter