netdev - Re: [PATCH v4 net] af_packet: Block execution of tasks waiting for transmit to complete in AF

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: <CAF=yD-+fCNGQyoRNAZngof3=_gGbHn9aSCQA_hNvFSsSZtZQxA@mail.gmail.com>
Date:   Tue, 25 Jun 2019 18:30:08 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Neil Horman <nhorman@...driver.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Matteo Croce <mcroce@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v4 net] af_packet: Block execution of tasks waiting for
 transmit to complete in AF_PACKET

> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index a29d66da7394..a7ca6a003ebe 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2401,6 +2401,9 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
>
>                 ts = __packet_set_timestamp(po, ph, skb);
>                 __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> +
> +               if (!packet_read_pending(&po->tx_ring))
> +                       complete(&po->skb_completion);
>         }
>
>         sock_wfree(skb);
> @@ -2585,7 +2588,7 @@ static int tpacket_parse_header(struct packet_sock *po, void *frame,
>
>  static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>  {
> -       struct sk_buff *skb;
> +       struct sk_buff *skb = NULL;
>         struct net_device *dev;
>         struct virtio_net_hdr *vnet_hdr = NULL;
>         struct sockcm_cookie sockc;
> @@ -2600,6 +2603,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>         int len_sum = 0;
>         int status = TP_STATUS_AVAILABLE;
>         int hlen, tlen, copylen = 0;
> +       long timeo = 0;
>
>         mutex_lock(&po->pg_vec_lock);
>
> @@ -2646,12 +2650,21 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
>         if ((size_max > dev->mtu + reserve + VLAN_HLEN) && !po->has_vnet_hdr)
>                 size_max = dev->mtu + reserve + VLAN_HLEN;
>
> +       reinit_completion(&po->skb_completion);
> +
>         do {
>                 ph = packet_current_frame(po, &po->tx_ring,
>                                           TP_STATUS_SEND_REQUEST);
>                 if (unlikely(ph == NULL)) {
> -                       if (need_wait && need_resched())
> -                               schedule();
> +                       if (need_wait && skb) {
> +                               timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> +                               timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);

This looks really nice.

But isn't it still susceptible to the race where tpacket_destruct_skb
is called in between po->xmit and this
wait_for_completion_interruptible_timeout?

The test for skb is shorthand for packet_read_pending  != 0, right?

> +                               if (timeo <= 0) {
> +                                       err = !timeo ? -ETIMEDOUT : -ERESTARTSYS;
> +                                       goto out_put;
> +                               }
> +                       }
> +                       /* check for additional frames */
>                         continue;
>                 }
>