netdev - Re: [PATCH v3] net: UDP repair mode for retrieving the send queue of corked UDP socket

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: <f097781778f6c54a0e4fea31d6e925504280f42c.camel@redhat.com>
Date:   Tue, 14 Apr 2020 11:27:43 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Leşe Doru Călin 
        <lesedorucalin01@...il.com>, Leon Romanovsky <leon@...nel.org>,
        netdev@...r.kernel.org
Cc:     David Miller <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH v3] net: UDP repair mode for retrieving the send queue
 of corked UDP socket

Hi,

I'm sorry for the late feedback, but I have a few comments...

On Tue, 2020-04-14 at 12:09 +0300, Leşe Doru Călin wrote:
> In this year's edition of GSoC, there is a project idea for CRIU to add 
> support for checkpoint/restore of cork-ed UDP sockets. But to add it, the
> kernel API needs to be extended.
> 
> This is what this patch does. It adds UDP "repair mode" for UDP sockets in 
> a similar approach to the TCP "repair mode", but only the send queue is
> necessary to be retrieved. So the patch extends the recv and setsockopt 
> syscalls. Using UDP_REPAIR option in setsockopt, caller can set the socket
> in repair mode. If it is setted, the recv/recvfrom/recvmsg will receive the
> write queue and the destination of the data. As in the TCP mode, to change 
> the repair mode requires the CAP_NET_ADMIN capability and to receive data 
> the caller is obliged to use the MSG_PEEK flag.
> 
> Signed-off-by: Lese Doru Calin <lesedorucalin01@...il.com>
> ---
[...]
@@ -1739,6 +1763,12 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
>  	if (flags & MSG_ERRQUEUE)
>  		return ip_recv_error(sk, msg, len, addr_len);
>  
> +	if (unlikely(up->repair)) {
> +		if (!peeking)
> +			return -EPERM;
> +		goto recv_sndq;
> +	}

If the code deduplication suggested below applies, perhaps you can
avoid the 'goto' statement and keep the 'repair' related code isolated
in a single place.

[...]

> 
>  	cond_resched();
>  	msg->msg_flags &= ~MSG_TRUNC;
>  	goto try_again;
> +
> +recv_sndq:
> +	off = sizeof(struct iphdr) + sizeof(struct udphdr);
> +	if (sin) {
> +		fl4 = &inet->cork.fl.u.ip4;
> +		sin->sin_family = AF_INET;
> +		sin->sin_port = fl4->fl4_dport;
> +		sin->sin_addr.s_addr = fl4->daddr;
> +		memset(sin->sin_zero, 0, sizeof(sin->sin_zero));
> +		*addr_len = sizeof(*sin);

Here the BPF_CGROUP_UDP4_RECVMSG hook is not invoked, why?

Perhaps you can reduce code duplication moving the esisting sin addr
handling to an helper, taking addr and port as arguments, and re-using
it here.

[...]

> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 7d4151747340..ec653f9fce2d 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -250,6 +250,28 @@ struct sock *udp6_lib_lookup(struct net *net, const struct in6_addr *saddr, __be
>  EXPORT_SYMBOL_GPL(udp6_lib_lookup);
>  #endif
>  
> +static int udp6_peek_sndq(struct sock *sk, struct msghdr *msg, int off, int len)
> +{
> +	int copy, copied = 0, err = 0;
> +	struct sk_buff *skb;
> +
> +	skb_queue_walk(&sk->sk_write_queue, skb) {
> +		copy = len - copied;
> +		if (copy > skb->len - off)
> +			copy = skb->len - off;
> +
> +		err = skb_copy_datagram_msg(skb, off, msg, copy);
> +		if (err)
> +			break;
> +
> +		copied += copy;
> +
> +		if (len <= copied)
> +			break;
> +	}
> +	return err ?: copied;
> +}
> +

This looks identical to the v4 version to me. If so, you could re-use
the same helper, exporting it.

Cheers,

Paolo