netdev - Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit path if no tx napi

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: <5352c98a-fa48-fcf9-c062-9986a317a1b0@redhat.com>
Date:   Mon, 21 Aug 2017 20:33:12 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Koichiro Den <den@...ipeden.com>, mst@...hat.com
Cc:     virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] virtio-net: invoke zerocopy callback on xmit
 path if no tx napi



On 2017年08月19日 14:38, Koichiro Den wrote:
> Facing the possible unbounded delay relying on freeing on xmit path,
> we also better to invoke and clear the upper layer zerocopy callback
> beforehand to keep them from waiting for unbounded duration in vain.
> For instance, this removes the possible deadlock in the case that the
> upper layer is a zerocopy-enabled vhost-net.
> This does not apply if napi_tx is enabled since it will be called in
> reasonale time.
>
> Signed-off-by: Koichiro Den <den@...ipeden.com>
> ---
>   drivers/net/virtio_net.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4302f313d9a7..f7deaa5b7b50 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1290,6 +1290,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
>   
>   	/* Don't wait up for transmitted skbs to be freed. */
>   	if (!use_napi) {
> +		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
> +			struct ubuf_info *uarg;
> +			uarg = skb_shinfo(skb)->destructor_arg;
> +			if (uarg->callback)
> +			    uarg->callback(uarg, true);
> +			skb_shinfo(skb)->destructor_arg = NULL;
> +			skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> +		}
>   		skb_orphan(skb);
>   		nf_reset(skb);
>   	}


Interesting, deadlock could be treated as a a radical case of the 
discussion here https://patchwork.kernel.org/patch/3787671/.

git grep tells more similar skb_orphan() cases. Do we need to change 
them all (or part)?

Actually, we may meet similar issues at many other places (e.g netem). 
Need to consider a complete solution for this. Figuring out all places 
that could delay a packet is a method.

Thanks