On 05/05/2015 11:26 AM, Tobias Klauser wrote:
On 2015-05-02 at 22:54:50 +0200, Vadim Kochan <[email protected]> wrote:
On Sat, May 02, 2015 at 07:00:53PM +0200, Tobias Klauser wrote:
On 2015-04-29 at 21:18:24 +0200, Vadim Kochan <[email protected]> wrote:
From: Vadim Kochan <[email protected]>

It might be related to the issue #73 noticed by

        Jon Schipp <[email protected]>

where he pointed that netsniff-ng captures some extra packets.

I observed this issue when I captured few Ethernet frames from
wireless device while I was sniffing Netlink monitor device (!!!),
especially under high load traffic (HD Video).

It can be easy grepped:

        sudo netsniff-ng/netsniff-ng -i nlmon0 --less | grep <dev>

where <dev> is the active high traffic device.

Interesting observation was that issue appeared only in the beginning of
the sniffing, and after some investigation I came up to the conclusion
that issue might be caused when netsniff-ng creates RX ring through the
setsockopt where Linux registers device hook for any device (we did not
bind yet socket to the specified device), and I assumed that before
binding to the specified device Linux can caught for us some packets
from the other devices until netsniff-ng will bind socket to the
specified device.

So fixed (I can't reproduce this anymore) by moving device binding
before attaching RX ring to the socket, in this case setsockopt consider
the bound device.

Signed-off-by: Vadim Kochan <[email protected]>
---
  ring_rx.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ring_rx.c b/ring_rx.c
index 32d3f6d..56685e0 100644
--- a/ring_rx.c
+++ b/ring_rx.c
@@ -231,10 +231,10 @@ void ring_rx_setup(struct ring *ring, int sock, size_t 
size, int ifindex,
  {
        fmemset(ring, 0, sizeof(*ring));
        setup_rx_ring_layout(sock, ring, size, jumbo_support, v3);
+       bind_ring_generic(sock, ring, ifindex, false);
        create_rx_ring(sock, ring, verbose);
        mmap_ring_generic(sock, ring);
        alloc_rx_ring_frames(sock, ring);
-       bind_ring_generic(sock, ring, ifindex, false);
        join_fanout_group(sock, fanout_group, fanout_type);
        prepare_polling(sock, poll);

Shouldn't mmap_ring_generic and alloc_rx_ring_frames be done before
bind_ring_generic? How about moving create_rx_ring down instead of
moving bind_ring_generic up?

Daniel can probably comment better on how exactly the order should be
and what the interdependecies are... Any comments?

My understanding is, that bind-ing in this case is just re-registering packet 
hook to
the specified device.

What would be a difference if after pf_alloc socket already has a
default packet hook which is bound to all devices and any protocol ?

Sure, Daniel's comments would great to have here :-)

Daniel, it seems the previous mail went to a wrong RH
address and bounced. Putting your iogearbox address to Cc

(Yes, don't work there anymore, so that email address is deprecated. ;))

Any comment from you on the order of
pf_socket()/bind()/create_ring()? You've certainly got more expertice
than me in that area :)

As the code currently is, it also conforms to the order suggested in [1]

Hehe, that does somehow sound familiar.

Okay, let me try to explain: Vadim's/Jon's observation is correct.

In the kernel, on socket creation time, we call into packet_create()
when we do pf_socket(). pf_socket() allocates the socket resource with
ETH_P_ALL. That means in packet_create() we already install ourselves
into the kernel's ptype_all list for taps via register_prot_hook(sk),
which eventually calls dev_add_pack().

Now, the thing is that when we actually do the bind in netsniff-ng with
ETH_P_ALL and ifindex 0, we save ourselves a second dev_add_pack() and
along with that an expensive unregister_prot_hook() which needs to wait
for a synchronize_net() barrier.

That said, the fix is not fully correct, as packet could still land
in the non-mmaped socket receive queue which is then never drained (and
thus skbs lingering there will take up/account for receive space which
is bad).

The better fix would just be to consolidate pf_socket() and pf_tx_socket()
and allocate both with socket(PF_PACKET, SOCK_RAW, 0). That way, we
should also be able to avoid the synchronize_net() barrier (as po->running
is false at bind time).

If we change pf_socket(), the only thing we need to make sure is that
all such sockets do a proper bind() call before starting to capture
packets, so that we don't break anything.

Cheers,
Daniel

[1] https://www.kernel.org/doc/Documentation/networking/packet_mmap.txt

--
You received this message because you are subscribed to the Google Groups 
"netsniff-ng" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to [email protected].
For more options, visit https://groups.google.com/d/optout.

Reply via email to