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;
Date:   Fri, 18 Nov 2022 14:44:17 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Hui Tang <tanghui20@huawei.com>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        mw@semihalf.com, linux@armlinux.org.uk, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
        yusongping@huawei.com
Subject: Re: [PATCH net v2] net: mdio-ipq4019: fix possible invalid pointer
 dereference
Message-ID: <Y3eMMc7maaPCKUNS@lunn.ch>
References: <20221117090514.118296-1-tanghui20@huawei.com>
 <Y3Y94/My9Al4pw+h@lunn.ch>
 <6cad3105-0e70-d890-162b-513855885fde@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <6cad3105-0e70-d890-162b-513855885fde@huawei.com>
Precedence: bulk

> So, the code should be as follows, is that right?
> 
> +	void __iomem *devm_ioremap_resource_optional(struct device *dev,
> +                                    	     const struct resource *res)
> +	{
> +		void __iomem *base;
> +
> +		base = __devm_ioremap_resource(dev, res, DEVM_IOREMAP);
> +		if (IS_ERR(base) && PTR_ERR(base) == -ENOMEM)
> +			return NULL;
> +
> +		return base;
> +	}
> 
> 
> [...]
> 	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> -	if (res)
> +	if (res) {
> +		priv->eth_ldo_rdy = devm_ioremap_resource_optional(&pdev->dev, res)
> +		if (IS_ERR(priv->eth_ldo_rdy))
> +			return PTR_ERR(priv->eth_ldo_rdy);
> +	}
> [...]

Yes, that is the basic concept.

The only thing i might change is the double meaning of -ENOMEM.
__devm_ioremap_resource() allocates memory, and if that memory
allocation fails, it returns -ENOMEM. If the resource does not exist,
it also returns -ENOMEM. So you cannot tell these two error conditions
apart. Most of the other get_foo() calls return -ENODEV if the
gpio/regulator/clock does not exist, so you can tell if you are out of
memory. But ioremap is specifically about memory so -ENOMEM actually
makes sense.

If you are out of memory, it seems likely the problem is not going to
go away quickly, so the next allocation will also fail, and hopefully
the error handling will then work. So i don't think it is major
issue. So yes, go with the code above.

      Andrew