Re: [PATCH net] net: ethtool: Don't call .cleanup_data when prepare_data fails

[Maxime Chevallier]

Hi Jakub,

On Fri, 4 Apr 2025 07:47:44 -0700
Jakub Kicinski <kuba@xxxxxxxxxx> wrote:

> On Thu,  3 Apr 2025 15:24:46 +0200 Maxime Chevallier wrote:
> > There's a consistent pattern where the .cleanup_data() callback is
> > called when .prepare_data() fails, when it should really be called to
> > clean after a successfull .prepare_data() as per the documentation.
> > 
> > Rewrite the error-handling paths to make sure we don't cleanup
> > un-prepared data.  
> 
> Code looks good. I have a question about the oldest instance of 
> the problem tho. The callbacks Michal added seem to be "idempotent".
> As you say the code doesn't implement the documented model, but
> I think until eeprom (?) was added the prepare callbacks could
> have only failed on memory allocation, and all the cleanup did
> was kfree(). So since kfree(NULL) is fine - nothing would have
> crashed..
> 
> Could you repost with the Fixes tag and an explanation of where
> the first instance of this causing a potential real crash was added?

TBH I didn't even see a crash, I just stumbled upon that when working
on the phy-dump stuff. I was actually surprised that I could trace it
back so far, surely things would've blown-up somewhere in the past 6
years...

I'll look at the chronology and see what was the first point in time
where a crash could've realistically gone wrong then.

Thanks

Maxime