netdev - Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <798df2d7-b13f-482a-8d4a-106c6492af01@app.fastmail.com>
Date: Thu, 18 Apr 2024 21:46:18 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Heiko Carstens" <hca@...ux.ibm.com>,
 "Nathan Chancellor" <nathan@...nel.org>
Cc: "Andrew Morton" <akpm@...ux-foundation.org>, gor@...ux.ibm.com,
 "Alexander Gordeev" <agordeev@...ux.ibm.com>,
 "Christian Borntraeger" <borntraeger@...ux.ibm.com>,
 "Sven Schnelle" <svens@...ux.ibm.com>, wintera@...ux.ibm.com,
 twinkler@...ux.ibm.com, linux-s390@...r.kernel.org,
 Netdev <netdev@...r.kernel.org>, llvm@...ts.linux.dev,
 patches@...ts.linux.dev
Subject: Re: [PATCH 1/3] s390/vmlogrdr: Remove function pointer cast

On Thu, Apr 18, 2024, at 17:15, Heiko Carstens wrote:
>> > > > -		/*
>> > > > -		 * The release function could be called after the
>> > > > -		 * module has been unloaded. It's _only_ task is to
>> > > > -		 * free the struct. Therefore, we specify kfree()
>> > > > -		 * directly here. (Probably a little bit obfuscating
>> > > > -		 * but legitime ...).
>> > > > -		 */
>> > > 
>> > > Why is the comment not relevant after this change? Or better: why is it not
>> > > valid before this change, which is why the code was introduced a very long
>> > > time ago? Any reference?
>> > > 
>> > > I've seen the warning since quite some time, but didn't change the code
>> > > before sure that this doesn't introduce the bug described in the comment.
>> > 
>> > From only 20 years ago:
>> > 
>> > https://lore.kernel.org/all/20040316170812.GA14971@kroah.com/
>> > 
>> > The particular code (zfcp) was changed, so it doesn't have this code
>> > (or never did?)  anymore, but for the rest this may or may not still
>> > be valid.
>> 
>> I guess relevant may not have been the correct word. Maybe obvious? I
>> can keep the comment but I do not really see what it adds, although
>> reading the above thread, I suppose it was added as justification for
>> calling kfree() as ->release() for a 'struct device'? Kind of seems like
>> that ship has sailed since I see this all over the place as a
>> ->release() function. I do not see how this patch could have a function
>> change beyond that but I may be misreading or misinterpreting your full
>> comment.
>
> That doesn't answer my question what prevents the release function
> from being called after the module has been unloaded.
>
> At least back then when the code was added it was a real bug.

I think the way this should work is to have the allocation and
the release function in the iucv bus driver, with a function
roughly like

struct device *iucv_alloc_device(char *name,
               const struct attribute_group *attrs,
               void *priv)
{
      dev = kzalloc(sizeof(struct device), GFP_KERNEL);
      if (!dev)
           return NULL;

      dev_set_name(dev, "%s", name);
      dev->bus = &iucv_bus;
      dev->parent = iucv_root;
      dev->groups = attrs;
      dev_set_drvdata(dev, priv);
      dev->release = iucv_free_dev;
  
      return dev;
}

Now the release function cannot go away as long as any module
is loaded that links against it, and those modules cannot
go away as long as the devices are in use.

I don't remember how iucv works, but if there is a way to
detect which system services exist, then the actual device
creation should also be separate from the driver using those
services, with another driver responsible for enumerating
the existing services and creating those devices.

      Arnd