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  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]
Date:   Sun, 23 Jun 2019 07:34:22 -0400
From:   Neil Horman <nhorman@...driver.com>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        Matteo Croce <mcroce@...hat.com>,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH v2 net] af_packet: Block execution of tasks waiting for
 transmit to complete in AF_PACKET

On Sat, Jun 22, 2019 at 10:12:46PM -0400, Willem de Bruijn wrote:
> On Sat, Jun 22, 2019 at 1:42 PM Neil Horman <nhorman@...driver.com> wrote:
> >
> > When an application is run that:
> > a) Sets its scheduler to be SCHED_FIFO
> > and
> > b) Opens a memory mapped AF_PACKET socket, and sends frames with the
> > MSG_DONTWAIT flag cleared, its possible for the application to hang
> > forever in the kernel.  This occurs because when waiting, the code in
> > tpacket_snd calls schedule, which under normal circumstances allows
> > other tasks to run, including ksoftirqd, which in some cases is
> > responsible for freeing the transmitted skb  (which in AF_PACKET calls a
> > destructor that flips the status bit of the transmitted frame back to
> > available, allowing the transmitting task to complete).
> >
> > However, when the calling application is SCHED_FIFO, its priority is
> > such that the schedule call immediately places the task back on the cpu,
> > preventing ksoftirqd from freeing the skb, which in turn prevents the
> > transmitting task from detecting that the transmission is complete.
> >
> > We can fix this by converting the schedule call to a completion
> > mechanism.  By using a completion queue, we force the calling task, when
> > it detects there are no more frames to send, to schedule itself off the
> > cpu until such time as the last transmitted skb is freed, allowing
> > forward progress to be made.
> >
> > Tested by myself and the reporter, with good results
> >
> > Appies to the net tree
> >
> > Signed-off-by: Neil Horman <nhorman@...driver.com>
> > Reported-by: Matteo Croce <mcroce@...hat.com>
> > CC: "David S. Miller" <davem@...emloft.net>
> > CC: Willem de Bruijn <willemdebruijn.kernel@...il.com>
> >
> > Change Notes:
> >
> > V1->V2:
> >         Enhance the sleep logic to support being interruptible and
> > allowing for honoring to SK_SNDTIMEO (Willem de Bruijn)
> > ---
> >  net/packet/af_packet.c | 60 ++++++++++++++++++++++++++++++++----------
> >  net/packet/internal.h  |  2 ++
> >  2 files changed, 48 insertions(+), 14 deletions(-)
> >
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index a29d66da7394..8ddb2f7aebb4 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -358,7 +358,8 @@ static inline struct page * __pure pgv_to_page(void *addr)
> >         return virt_to_page(addr);
> >  }
> >
> > -static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> > +static void __packet_set_status(struct packet_sock *po, void *frame, int status,
> > +                               bool call_complete)
> >  {
> >         union tpacket_uhdr h;
> >
> > @@ -381,6 +382,8 @@ static void __packet_set_status(struct packet_sock *po, void *frame, int status)
> >                 BUG();
> >         }
> >
> > +       if (po->wait_on_complete && call_complete)
> > +               complete(&po->skb_completion);
> 
> This wake need not happen before the barrier. Only one caller of
> __packet_set_status passes call_complete (tpacket_destruct_skb).
> Moving this branch to the caller avoids a lot of code churn.
> 
Yeah, thats a fair point, I'll adjust that.

> Also, multiple packets may be released before the process is awoken.
> The process will block until packet_read_pending drops to zero. Can
> defer the wait_on_complete to that one instance.
> 
Also true, we can gate the complete call on wait_on_complete and
pack_read_pending(...) == 0
> >         smp_wmb();
> >  }
> >
> > @@ -1148,6 +1151,14 @@ static void *packet_previous_frame(struct packet_sock *po,
> >         return packet_lookup_frame(po, rb, previous, status);
> >  }
> >
> > +static void *packet_next_frame(struct packet_sock *po,
> > +               struct packet_ring_buffer *rb,
> > +               int status)
> > +{
> > +       unsigned int next = rb->head != rb->frame_max ? rb->head+1 : 0;
> > +       return packet_lookup_frame(po, rb, next, status);
> > +}
> > +
> >  static void packet_increment_head(struct packet_ring_buffer *buff)
> >  {
> >         buff->head = buff->head != buff->frame_max ? buff->head+1 : 0;
> > @@ -2360,7 +2371,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
> >  #endif
> >
> >         if (po->tp_version <= TPACKET_V2) {
> > -               __packet_set_status(po, h.raw, status);
> > +               __packet_set_status(po, h.raw, status, false);
> >                 sk->sk_data_ready(sk);
> >         } else {
> >                 prb_clear_blk_fill_status(&po->rx_ring);
> > @@ -2400,7 +2411,7 @@ static void tpacket_destruct_skb(struct sk_buff *skb)
> >                 packet_dec_pending(&po->tx_ring);
> >
> >                 ts = __packet_set_timestamp(po, ph, skb);
> > -               __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts);
> > +               __packet_set_status(po, ph, TP_STATUS_AVAILABLE | ts, true);
> >         }
> >
> >         sock_wfree(skb);
> > @@ -2585,13 +2596,13 @@ 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;
> >         __be16 proto;
> >         int err, reserve = 0;
> > -       void *ph;
> > +       void *ph = NULL;
> >         DECLARE_SOCKADDR(struct sockaddr_ll *, saddr, msg->msg_name);
> >         bool need_wait = !(msg->msg_flags & MSG_DONTWAIT);
> >         unsigned char *addr = NULL;
> > @@ -2600,9 +2611,12 @@ 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;
> >
> >         mutex_lock(&po->pg_vec_lock);
> >
> > +       timeo = sock_sndtimeo(&po->sk, msg->msg_flags & MSG_DONTWAIT);
> > +
> >         if (likely(saddr == NULL)) {
> >                 dev     = packet_cached_dev_get(po);
> >                 proto   = po->num;
> > @@ -2647,16 +2661,29 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >                 size_max = dev->mtu + reserve + VLAN_HLEN;
> >
> >         do {
> > +
> > +               if (po->wait_on_complete && need_wait) {
> > +                       timeo = wait_for_completion_interruptible_timeout(&po->skb_completion, timeo);
> 
> Why move the sleeping location from where it was with schedule()?
> Without that change, ph and skb are both guaranteed to be NULL after
> packet_current_frame, so can jump to out_put and avoid adding branches
> at out_status. And no need for packet_next_frame.
> 
> Just replace schedule with a sleeping function in place (removing the
> then obsolete need_resched call).
> 
> That is a much shorter patch and easier to compare for correctness
> with the current code.
> 
> minor: probably preferable to first check local variable need_wait
> before reading a struct member.
> 
> > +                       po->wait_on_complete = 0;
> > +                       if (!timeo) {
> > +                               /* We timed out, break out and notify userspace */
> > +                               err = -ETIMEDOUT;
> > +                               goto out_status;
> > +                       } else if (timeo == -ERESTARTSYS) {
> > +                               err = -ERESTARTSYS;
> > +                               goto out_status;
> > +                       }
> > +               }
> > +
> >                 ph = packet_current_frame(po, &po->tx_ring,
> >                                           TP_STATUS_SEND_REQUEST);
> > -               if (unlikely(ph == NULL)) {
> > -                       if (need_wait && need_resched())
> > -                               schedule();
> > -                       continue;
> > -               }
> > +
> > +               if (likely(ph == NULL))
> 
> why switch from unlikely to likely?
> 
> > +                       break;
> >
> >                 skb = NULL;
> >                 tp_len = tpacket_parse_header(po, ph, size_max, &data);
> > +
> 
> nit: irrelevant
> 
> >                 if (tp_len < 0)
> >                         goto tpacket_error;
> >
> > @@ -2699,7 +2726,7 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >  tpacket_error:
> >                         if (po->tp_loss) {
> >                                 __packet_set_status(po, ph,
> > -                                               TP_STATUS_AVAILABLE);
> > +                                               TP_STATUS_AVAILABLE, false);
> >                                 packet_increment_head(&po->tx_ring);
> >                                 kfree_skb(skb);
> >                                 continue;
> > @@ -2719,7 +2746,9 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >                 }
> >
> >                 skb->destructor = tpacket_destruct_skb;
> > -               __packet_set_status(po, ph, TP_STATUS_SENDING);
> > +               __packet_set_status(po, ph, TP_STATUS_SENDING, false);
> > +               if (!packet_next_frame(po, &po->tx_ring, TP_STATUS_SEND_REQUEST))
> > +                       po->wait_on_complete = 1;
> >                 packet_inc_pending(&po->tx_ring);
> >
> >                 status = TP_STATUS_SEND_REQUEST;
> > @@ -2753,8 +2782,10 @@ static int tpacket_snd(struct packet_sock *po, struct msghdr *msg)
> >         goto out_put;
> >
> >  out_status:
> > -       __packet_set_status(po, ph, status);
> > -       kfree_skb(skb);
> > +       if (ph)
> > +               __packet_set_status(po, ph, status, false);
> > +       if (skb)
> > +               kfree_skb(skb);
> >  out_put:
> >         dev_put(dev);
> >  out:
> > @@ -3207,6 +3238,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
> >         sock_init_data(sock, sk);
> >
> >         po = pkt_sk(sk);
> > +       init_completion(&po->skb_completion);
> >         sk->sk_family = PF_PACKET;
> >         po->num = proto;
> >         po->xmit = dev_queue_xmit;
> > diff --git a/net/packet/internal.h b/net/packet/internal.h
> > index 3bb7c5fb3bff..bbb4be2c18e7 100644
> > --- a/net/packet/internal.h
> > +++ b/net/packet/internal.h
> > @@ -128,6 +128,8 @@ struct packet_sock {
> >         unsigned int            tp_hdrlen;
> >         unsigned int            tp_reserve;
> >         unsigned int            tp_tstamp;
> > +       struct completion       skb_completion;
> > +       unsigned int            wait_on_complete;
> 
> Probably belong in packet_ring_buffer. Near pending_refcnt as touched
> in the same code. And because protected by the ring buffer mutex.
> 
> 
> 
> >         struct net_device __rcu *cached_dev;
> >         int                     (*xmit)(struct sk_buff *skb);
> >         struct packet_type      prot_hook ____cacheline_aligned_in_smp;
> > --
> > 2.20.1
> >
> 

Powered by blists - more mailing lists