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: <20240419121506.23824-A-hca@linux.ibm.com>
Date: Fri, 19 Apr 2024 14:15:06 +0200
From: Heiko Carstens <hca@...ux.ibm.com>
To: Arnd Bergmann <arnd@...db.de>, Alexandra Winter <wintera@...ux.ibm.com>,
        Thorsten Winkler <twinkler@...ux.ibm.com>
Cc: Nathan Chancellor <nathan@...nel.org>,
        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 09:46:18PM +0200, Arnd Bergmann wrote:
> 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 ...).
> >> > > > -		 */
> 
> 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.

I have the impression we have the same discussion like it happened 20
years ago:
https://lore.kernel.org/all/OF876C2271.59086B92-ONC1256E5A.00409933-C1256E5A.00427853@de.ibm.com/

Adding extra module dependencies won't help, since it just moves the
potential races. However what could easily solve this problem is to
make CONFIG_IUCV a boolean config option instead of tristate. If it
would be compiled in, the release function cannot go away.

We have already "def_tristate y if S390" for IUCV, so it looks like
this wouldn't change anything in real life. In addition with something
like your proposed change, we should be fine.

Plus we need to fix the potential bug you introduced with commit
42af6bcbc351 ("tty: hvc-iucv: fix function pointer casts"). But at
least this is also iucv_bus related.

Alexandra, Thorsten, any objections if CONFIG_IUCV would be changed so
it can only be compiled in or out, but not as a module anymore?