Re: [PATCH 03/13 net-next] net: inet: relocate ip_generic_getfrag and guard IPv4 socket logic

[Fernando Fernandez Mancera]

On 9/10/26 11:19 PM, Stanislav Fomichev wrote:
> On 09/10, Fernando Fernandez Mancera wrote:
> > To enable compiling the INET subsystem without IPv4, shared generic
> > utilities must be relocated and IPv4 socket logic must be guarded for
> > CONFIG_IPV4.
> >
> > This patch moves the generic ip_generec_getfrag() from ip_output.c to
> > af_inet.c. It also introduces CONFIG_IPV4 guards around af_inet.c to
> > reject IPv4-specific ioctls, protocol registrations and bind requests.
> > The same guard is added to reject IPv4-mapped IPv6.
> >
> > Signed-off-by: Fernando Fernandez Mancera <fmancera@xxxxxxx>
> > ---
> >   net/ipv4/af_inet.c   | 96 +++++++++++++++++++++++++++++++++++++-------
> >   net/ipv4/ip_output.c | 18 ---------
> >   net/ipv6/af_inet6.c  |  5 +++
> >   net/ipv6/datagram.c  | 12 ++++++
> >   4 files changed, 99 insertions(+), 32 deletions(-)
> >
> > diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> > index d9421ac38d78..b0c48ba544bf 100644
> > --- a/net/ipv4/af_inet.c
> > +++ b/net/ipv4/af_inet.c
> > @@ -129,6 +129,28 @@
> >   int disable_ipv6_mod;
> >   EXPORT_SYMBOL(disable_ipv6_mod);
> >   
> > +/* Keep the function here for now as it is generic, it should be moved
> > + * to a common L3 place
> > + */
> > +int
> > +ip_generic_getfrag(void *from, char *to, int offset, int len, int odd, struct sk_buff *skb)
> > +{
> > +	struct msghdr *msg = from;
> > +
> > +	if (skb->ip_summed == CHECKSUM_PARTIAL) {
> > +		if (!copy_from_iter_full(to, len, &msg->msg_iter))
> > +			return -EFAULT;
> > +	} else {
> > +		__wsum csum = 0;
> > +
> > +		if (!csum_and_copy_from_iter_full(to, len, &csum, &msg->msg_iter))
> > +			return -EFAULT;
> > +		skb->csum = csum_block_add(skb->csum, csum, odd);
> > +	}
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(ip_generic_getfrag);
> > +
> >   /* The inetsw table contains everything that inet_create needs to
> >    * build a new socket.
> >    */
> > @@ -425,8 +447,10 @@ int inet_release(struct socket *sock)
> >   		if (!sk->sk_kern_sock)
> >   			BPF_CGROUP_RUN_PROG_INET_SOCK_RELEASE(sk);
> >   
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   		/* Applications forget to leave groups before exiting */
> >   		ip_mc_drop_socket(sk);
> > +#endif
> >   
> >   		/* If linger is set, we don't return until the close
> >   		 * is complete.  Otherwise we return immediately. The
> > @@ -478,6 +502,7 @@ EXPORT_SYMBOL(inet_bind);
> >   int __inet_bind(struct sock *sk, struct sockaddr_unsized *uaddr, int addr_len,
> >   		u32 flags)
> >   {
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   	struct sockaddr_in *addr = (struct sockaddr_in *)uaddr;
> >   	struct inet_sock *inet = inet_sk(sk);
> >   	struct net *net = sock_net(sk);
> > @@ -570,6 +595,9 @@ int __inet_bind(struct sock *sk, struct sockaddr_unsized *uaddr, int addr_len,
> >   		release_sock(sk);
> >   out:
> >   	return err;
> > +#else
> > +	return -EAFNOSUPPORT;
> > +#endif
> >   }
> >   
> >   int inet_dgram_connect(struct socket *sock, struct sockaddr_unsized *uaddr,
> > @@ -962,18 +990,24 @@ EXPORT_SYMBOL(inet_shutdown);
> >   int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
> >   {
> >   	struct sock *sk = sock->sk;
> > -	int err = 0;
> > -	struct net *net = sock_net(sk);
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   	void __user *p = (void __user *)arg;
> > -	struct ifreq ifr;
> > +	struct net *net = sock_net(sk);
> >   	struct rtentry rt;
> > +	struct ifreq ifr;
> > +#endif
> > +	int err = 0;
> >   
> >   	switch (cmd) {
> >   	case SIOCADDRT:
> >   	case SIOCDELRT:
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   		if (copy_from_user(&rt, p, sizeof(struct rtentry)))
> >   			return -EFAULT;
> >   		err = ip_rt_ioctl(net, cmd, &rt);
> > +#else
> > +		err = -EOPNOTSUPP;
> > +#endif
> >   		break;
> >   	case SIOCRTMSG:
> >   		err = -EINVAL;
> > @@ -981,18 +1015,26 @@ int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
> >   	case SIOCDARP:
> >   	case SIOCGARP:
> >   	case SIOCSARP:
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   		err = arp_ioctl(net, cmd, (void __user *)arg);
> > +#else
> > +		err = -EOPNOTSUPP;
> > +#endif
> >   		break;
> >   	case SIOCGIFADDR:
> >   	case SIOCGIFBRDADDR:
> >   	case SIOCGIFNETMASK:
> >   	case SIOCGIFDSTADDR:
> >   	case SIOCGIFPFLAGS:
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   		if (get_user_ifreq(&ifr, NULL, p))
> >   			return -EFAULT;
> >   		err = devinet_ioctl(net, cmd, &ifr);
> >   		if (!err && put_user_ifreq(&ifr, p))
> >   			err = -EFAULT;
> > +#else
> > +		err = -EOPNOTSUPP;
> > +#endif
> >   		break;
> >   
> >   	case SIOCSIFADDR:
> > @@ -1001,9 +1043,13 @@ int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
> >   	case SIOCSIFDSTADDR:
> >   	case SIOCSIFPFLAGS:
> >   	case SIOCSIFFLAGS:
> > +#if IS_ENABLED(CONFIG_IPV4)
> >   		if (get_user_ifreq(&ifr, NULL, p))
> >   			return -EFAULT;
> >   		err = devinet_ioctl(net, cmd, &ifr);
> > +#else
> > +		err = -EOPNOTSUPP;
> > +#endif
>
> (passing by comment)
>
> Don't we have a coding style rule to avoid ifdef conditional in C code?
> Should we add some new devinet4_ioctl/etc wrappers that we can conditionally
> compile out in the headers?

I think yes but when it makes sense. Of course, that is something a bit 
hard to judge IMHO. I tried to use it when it makes sense, like for 
stubs or for functions that are protocol agnostic but contains a small 
part related to IPv4/IPV6. I was willing to avoid code duplication as 
much as possible.