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:40:21 -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:21:31PM -0400, Willem de Bruijn wrote:
> > > -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.
> >
> > 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.
> 
> Eh no. The point of having this sleep in the send loop is that
> additional slots may be released for transmission (flipped to
> TP_STATUS_SEND_REQUEST) from another thread while this thread is
> waiting.
> 
Thats incorrect.  The entirety of tpacket_snd is protected by a mutex. No other
thread can alter the state of the frames in the vector from the kernel send path
while this thread is waiting.

> Else, it would have been much simpler to move the wait below the send
> loop: send as many packets as possible, then wait for all of them
> having been released. Much clearer control flow.
> 
Thats (almost) what happens now.  The only difference is that with this
implementation, the waiting thread has the opportunity to see if userspace has
queued more frames for transmission during the wait period.  We could
potentially change that, but thats outside the scope of this fix.

> Where to set and clear the wait_on_complete boolean remains. Integer
> assignment is fragile, as the compiler and processor may optimize or
> move simple seemingly independent operations. As complete() takes a
> spinlock, avoiding that in the DONTWAIT case is worthwhile. But probably
> still preferable to set when beginning waiting and clear when calling
> complete.
We avoid any call to wait_for_complete or complete already, based on the gating
of the need_wait variable in tpacket_snd.  If the transmitting thread doesn't
set MSG_DONTWAIT in the flags of the msg structure, we will never set
wait_for_complete, and so we will never manipulate the completion queue.

Neil

> 

Powered by blists - more mailing lists