On Wed, Jun 26, 2019 at 6:54 AM Neil Horman <nhor...@tuxdriver.com> wrote:
>
> On Tue, Jun 25, 2019 at 06:30:08PM -0400, Willem de Bruijn wrote:
> > > 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?
> >
> Thats not an issue, since the complete is only gated on packet_read_pending
> reaching 0 in tpacket_destuct_skb. Previously it was gated on my
> wait_on_complete flag being non-zero, so we had to set that prior to calling
> po->xmit, or the complete call might never get made, resulting in a hang.
> Now,
> we will always call complete, and the completion api allows for arbitrary
> ordering of complete/wait_for_complete (since its internal done variable gets
> incremented), making a call to wait_for_complete effectively a fall through if
> complete gets called first.
Perfect! I was not aware that it handles that internally. Hadn't read
do_wait_for_common closely before.
> There is an odd path here though. If an application calls sendmsg on a packet
> socket here with MSG_DONTWAIT set, then need_wait will be zero, and we will
> eventually exit this loop without ever having called wait_for_complete, but
> tpacket_destruct_skb will still have called complete when all the frames
> complete transmission. In and of itself, thats fine, but it leave the
> completion structure in a state where its done variable will have been
> incremented at least once (specifically it will be set to N, where N is the
> number of frames transmitted during the call where MSG_DONTWAIT is set). If
> the
> application then calls sendmsg on this socket with MSG_DONTWAIT clear, we will
> call wait_for_complete, but immediately return from it (due to the previously
> made calls to complete). I've corrected this however, but adding that call to
> reinit_completion prior to the loop entry, so that we are always guaranteed to
> have the completion variable set properly to wait for only the frames being
> sent
> in this particular instance of the sendmsg call.
Yep, understood.
>
> > The test for skb is shorthand for packet_read_pending != 0, right?
> >
> Sort of. gating on skb guarantees for us that we have sent at least one frame
> in this call to tpacket_snd. If we didn't do that, then it would be possible
> for an application to call sendmsg without setting any frames in the buffer to
> TP_STATUS_SEND_REQUEST, which would cause us to wait for a completion without
> having sent any frames, meaning we would block waiting for an event
> (tpacket_destruct_skb), that will never happen. The check for skb ensures
> that
> tpacket_snd_skb will get called, and that we will get a wakeup from a call to
> wait_for_complete. It does suggest that packet_read_pending != 0, but thats
> not
> guaranteed, because tpacket_destruct_skb may already have been called (see the
> above explination regarding ordering of complete/wait_for_complete).
But the inverse is true: if gating sleeping on packet_read_pending,
the process only ever waits if a packet is still to be acknowledged.
Then both the wait and wake clearly depend on the same state.
Either way works, I think. So this is definitely fine.
One possible refinement would be to keep po->wait_on_complete (but
rename as po->wake_om_complete), set it before entering the loop and
clear it before function return (both within the pg_vec_lock critical
section). And test that in tpacket_destruct_skb to avoid calling
complete if MSG_DONTWAIT. But I don't think it's worth the complexity.
One rare edge case is a MSG_DONTWAIT send followed by a !MSG_DONTWAIT.
It is then possible for a tpacket_destruct_skb to be run as a result
from the first call, during the second call, after the call to
reinit_completion. That would cause the next wait to return before
*its* packets have been sent. But due to the packet_read_pending test
in the while () condition it will loop again and return to wait. So that's fine.
Thanks for bearing with me.
Reviewed-by: Willem de Bruijn <will...@google.com>
