Re: [PATCH v3 phy-next 2/8] soc: fsl: guts: add a global structure to hold state

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Michael Walle <mwalle@kernel.org>
Cc: devicetree@vger.kernel.org, robh@kernel.org, conor+dt@kernel.org,
	vkoul@kernel.org, neil.armstrong@linaro.org, olteanv@gmail.com,
	linux-phy@lists.infradead.org
Subject: Re: [PATCH v3 phy-next 2/8] soc: fsl: guts: add a global structure to hold state
Date: Tue, 21 Jul 2026 13:43:32 +0300	[thread overview]
Message-ID: <20260721104332.e65ma6tiwoisgo7m@skbuf> (raw)
In-Reply-To: <DK44809N7Y8I.J2Z3U4N32H0Q@kernel.org>

Hi Michael,

On Tue, Jul 21, 2026 at 10:44:30AM +0200, Michael Walle wrote:
> Hi Vladimir,
> 
> On Mon Jul 20, 2026 at 4:57 PM CEST, Vladimir Oltean wrote:
> > On Mon, Jul 20, 2026 at 05:01:39PM +0300, Vladimir Oltean wrote:
> >> On Mon, Jul 20, 2026 at 01:50:49PM +0000, sashiko-bot@kernel.org wrote:
> >> > [Severity: High]
> >> > Will jumping to err_nomem cause a NULL pointer dereference?
> >> > 
> >> > If the allocation of soc_dev_attr fails, the code now jumps to the error
> >> > handling path which dereferences soc_dev_attr to free its members:
> >> > 
> >> > > @@ -269,6 +271,8 @@ static int __init fsl_guts_init(void)
> >> > >  	kfree(soc_dev_attr->revision);
> >> > >  	kfree(soc_dev_attr->serial_number);
> >> > 
> >> > Since soc_dev_attr is NULL when this allocation fails, won't evaluating
> >> > soc_dev_attr->revision and other members result in a kernel panic?
> >> 
> >> yeah.
> >> 
> >> I tried to be nice and play with the existing "err_nomem" and "err"
> >> error handling labels, but the truth is that a single error cleanup path
> >> is just an antipattern that doesn't scale as code becomes more complex.
> >> 
> >> Instead of having a single error handling procedure which needs to
> >> carefully fend off from various invalid contexts from all the goto
> >> sites, the standard convention is to have an incremental set of labels
> >> which undo just the setup that was done up to the goto jump site.
> >> 
> >> I'll refactor fsl_guts_init() to use that convention for v4.
> >
> > While I was doing this refactoring, I noticed that this code path:
> >
> > 	if (soc_data)
> > 		soc_uid = fsl_guts_get_soc_uid(soc_data->sfp_compat,
> > 					       soc_data->uid_offset);
> > 	if (soc_uid) {
> > 		soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX",
> > 							soc_uid);
> > 		// no kasprintf() NULL return code check here, unlike
> > 		// the rest of the code
> 
> It's a long time ago, but it seems to be designed be optional. In
> that case a NULL pointer check was omitted, because nothing bad can
> happen anyway. Either you'll free the string if != NULL or you don't
> free anything.
> 
> > 	}
> >
> > 	soc_dev = soc_device_register(soc_dev_attr);
> > 	if (IS_ERR(soc_dev)) {
> > 		ret = PTR_ERR(soc_dev);
> > 		goto err;
> > 	}
> >
> > proceeds with soc_device_register() even if fsl_guts_get_soc_uid() was
> > able to get a soc unique id from the security fuse processor, but we
> > fail to print that string to the soc_dev_attr->serial_number variable.
> >
> > I don't see anywhere mentioned in commit 786dde1e59d7 ("soc: fsl: guts:
> > add serial_number support") that this would be intentional, so it can
> > just as well be an omission.
> >
> > Is it OK if I replace the silent failure with a loud failure of the
> > entire fsl_guts_init()? The code executes only if soc_uid is non-zero
> > anyway.
> >
> > kasprintf() fails only for memory related reasons.
> 
> I don't have a strong opinion. If you ever get that error, something
> is seriously bad anyway.

Thanks for responding.

I think the optionality is already taken care of by the "if (soc_uid)" check.
My intuition is that if the guts driver decides to try and populate the
soc_dev_attr->serial_number attribute, it should try and run this to
completion.

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy