Re: [PATCH net v3] net/iucv: require IUCV for AF_IUCV sockets

[Alexandra Winter <wintera@linux.ibm.com>]

On 21.09.26 05:48, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] net/iucv/Kconfig: replacing `depends on S390` with `depends on
>   IUCV` for AFIUCV reverts, without disclosure, the design intent of the…
> 
> --

Pengpeng Hou,
thank you for your contribution.

tl,dr: I think Sashiko is right and I found another issue with this patch when testing.
Let me know, if you want me to continue with a Reported-by you, or if you want to work
on a v4. In that case see my comments below.


> 
> net/iucv: require IUCV for AF_IUCV sockets
> 
> This changes the AFIUCV Kconfig symbol from "depends on S390" to
> "depends on IUCV" so that AFIUCV can no longer be enabled when the IUCV
> core is disabled. It replaces the v2 approach of always descending into
> net/iucv from net/Makefile.
> 
> Previous posting referenced in the patch:
> https://lore.kernel.org/all/20260815135456.53684-1-pengpeng@iscas.ac.cn/
> 
>> diff --git a/net/iucv/Kconfig b/net/iucv/Kconfig
>> index 5cfddc9c6498..2686d795cc2f 100644
>> --- a/net/iucv/Kconfig
>> +++ b/net/iucv/Kconfig
>> @@ -9,7 +9,7 @@ config IUCV
>>  	  communication link between VM guests.
>>  
>>  config AFIUCV
>> -	depends on S390
>> +	depends on IUCV
> 
> [Severity: Medium]
> Is the stated rationale accurate here?  The commit message says:
> 
>   "Independently entering the directory is not sufficient: AF_IUCV also
>    relies on the IUCV implementation."
> 
> Looking at afiucv_init() in net/iucv/af_iucv.c, the only reference to
> the IUCV core is compile-time guarded:
> 
> 	if (machine_is_vm() && IS_ENABLED(CONFIG_IUCV)) {
> 		...
> 		pr_iucv = &iucv_if;
> 	} else {
> 		memset(&iucv_userid, 0, sizeof(iucv_userid));
> 		pr_iucv = NULL;
> 	}
> 
> and iucv_sock_alloc() treats a NULL provider as a supported mode:
> 
> 	if (pr_iucv)
> 		iucv->transport = AF_IUCV_TRANS_IUCV;
> 	else
> 		iucv->transport = AF_IUCV_TRANS_HIPER;
> 
> The other iucv_* users in af_iucv.c are iucv_path_alloc() and
> iucv_path_free(), which are static inline wrappers around kzalloc_obj()
> and kfree() in include/net/iucv/iucv.h.  Does af_iucv.o then have any
> link-time dependency on the IUCV core at all with CONFIG_IUCV=n?
> 

I actually tested IUCV=N, AF_IUCV=M with your v2 and it worked fine.
So it's not that "AF_IUCV also relies on the IUCV implementation";

In case of IUCV=M, AF_IUCV=Y
it's that 'IS_ENABLED(CONFIG_IUCV)' causes the issue of
	undefined reference to `iucv_if'
when IUCV is a module and AF_IUCV is built in.



> The reason af_iucv.o is not built in that configuration appears to be
> only the traversal rule in net/Makefile:
> 
> 	obj-$(CONFIG_IUCV)		+= iucv/
> 
> Also, c69748d1c9b5, named in the Fixes: tag, deliberately changed
> "depends on IUCV" to "depends on S390" together with
> "def_tristate m if QETH_L3 || IUCV" so that AF_IUCV could be enabled for
> the HiperSockets transport alone.  Since this patch makes the AFIUCV
> symbol invisible for every CONFIG_IUCV=n configuration, is the word
> "Restore" describing the right direction, and could the commit message
> say explicitly that the HiperSockets-only configuration is being dropped
> rather than fixed?


I agree it seems, it was the intention, that AF_IUCV over HiperSockets,
could be used independently of IUCV over z/VM's TRANS_IUCV.
However I never heard of any usecase where such a kernel was built,
typically you want both on s390.
That's why I proposed the shortcut.


> 
> One further consequence worth spelling out: iucv_init() in
> net/iucv/iucv.c bails out early on non-z/VM systems:
> 
> 	if (!machine_is_vm()) {
> 		rc = -EPROTONOSUPPORT;
> 		goto out;
> 	}
> 
> so on an LPAR, does a modular IUCV still work as the provider that
> AFIUCV=m now depends on, or does this effectively force CONFIG_IUCV=y
> for HiperSockets AF_IUCV users?
> 


Actually even with your v2 IUCV=M, AF_IUCV=M builds fine in an LPAR,
but modprobe returns with
modprobe: ERROR: could not insert 'af_iucv': Protocol not supported	(not expected)
modprobe: ERROR: could not insert 'iucv': Protocol not supported   	(expected)

Is this a result of
> 	if (machine_is_vm() && ...
>		err = -EPROTONOSUPPORT;

I don't fully understand it yet.

But that combination can be defined, even with your v3, so we have another open issue here.

I definitively need to test all possible combinations! (Should have done that earlier, I admit)



>>  	def_tristate m if QETH_L3 || IUCV
>>  	prompt "AF_IUCV Socket support (S390 - z/VM and HiperSockets transport)"
>>  	help
> 
> [Severity: Low]
> With "depends on IUCV" in place, IUCV is always y or m whenever AFIUCV
> is visible, so the QETH_L3 term of
> 
> 	def_tristate m if QETH_L3 || IUCV
> 
> can never decide the default any more.  Should the default clause be
> reduced to just "def_tristate m"?
> 
> Similarly, the prompt and the help text below still present z/VM and
> HiperSockets as separable transports:
> 
> 	  Select this option if you want to use AF_IUCV socket applications
> 	  based on z/VM inter-user communication vehicle or based on
> 	  HiperSockets.
> 
> Do these need updating too, now that the HiperSockets-only case can no
> longer be configured?  The IS_ENABLED(CONFIG_IUCV) test in
> afiucv_init() also becomes permanently true after this change.

Valid thoughts, in case we go with the 'depends' option.