[PATCH?] Fix sniffing of ARP replies

[PATCH?] Fix sniffing of ARP replies

[Petr Vandrovec]

Hi Dave,
  after recent changes in packet_type interface I stopped setting
af_packet_priv - as you told that it is only for AF_PACKET, for nobody
else. And - things stopped working.

  After some debugging I found that there are skb's with ->sk == NULL -
namely ARP replies - and if af_packet_priv == NULL, these skbs are not
delivered to the registered ptype.

  Is change below correct, or should I simple set af_packet_priv to
non-NULL value, like I did before (and like I have to do it again for
older kernels, in the light of problems I'm seeing now)?
					Thanks,
						Petr Vandrovec


diff -urdN linux/net/core/dev.c linux/net/core/dev.c
--- linux/net/core/dev.c	2003-10-15 14:05:23.000000000 +0000
+++ linux/net/core/dev.c	2003-10-15 16:52:41.000000000 +0000
@@ -942,7 +942,8 @@
 		 * they originated from - MvS (miquels@drinkel.ow.org)
 		 */
 		if ((ptype->dev == dev || !ptype->dev) &&
-		    (struct sock *)ptype->af_packet_priv != skb->sk) {
+		    (ptype->af_packet_priv == NULL ||
+		     (struct sock *)ptype->af_packet_priv != skb->sk)) {
 			struct sk_buff *skb2= skb_clone(skb, GFP_ATOMIC);
 			if (!skb2)
 				break;

Re: [PATCH?] Fix sniffing of ARP replies

[David S. Miller]

On Wed, 15 Oct 2003 19:11:12 +0200
Petr Vandrovec <vandrove@vc.cvut.cz> wrote:

>   after recent changes in packet_type interface I stopped setting
> af_packet_priv - as you told that it is only for AF_PACKET, for nobody
> else. And - things stopped working.

"Where" did you stop doing this?

Nothing in the current kernel should be broken at all by said
changes I did to 2.6.x

If it has broken something you are working on external to the tree
you have to say what it is.  I can only guess that it's making
assumptions that never truly existed.

Re: [PATCH?] Fix sniffing of ARP replies

[David S. Miller]

On Wed, 15 Oct 2003 19:11:12 +0200
Petr Vandrovec <vandrove@vc.cvut.cz> wrote:

> -		    (struct sock *)ptype->af_packet_priv != skb->sk) {
> +		    (ptype->af_packet_priv == NULL ||
> +		     (struct sock *)ptype->af_packet_priv != skb->sk)) {

Ok, after studying this some more, I think this patch makes
sense and I'll apply it to 2.6.x

Thanks for being patient with my idiocy this afternoon.
:-)

Re: [PATCH?] Fix sniffing of ARP replies

[Petr Vandrovec]

On 15 Oct 03 at 20:47, David S. Miller wrote:
> On Wed, 15 Oct 2003 19:11:12 +0200
> Petr Vandrovec <vandrove@vc.cvut.cz> wrote:
> 
> >   after recent changes in packet_type interface I stopped setting
> > af_packet_priv - as you told that it is only for AF_PACKET, for nobody
> > else. And - things stopped working.
> 
> "Where" did you stop doing this?

vmnet. http://platan.vc.cvut.cz/ftp/pub/vmware/vmware-any-any-update42.tar.gz,
vmnet.tar -> bridge.c.
 
> Nothing in the current kernel should be broken at all by said
> changes I did to 2.6.x
> 
> If it has broken something you are working on external to the tree
> you have to say what it is.  I can only guess that it's making
> assumptions that never truly existed.

It behaves same way it behaved. But recently you renamed packet_type.data
to packet_type.af_packet_priv, saying that af_packet_priv should be used
only by AF_PACKET code, by nobody else. So I trusted you, removed
packet_type.data (and packet_type.af_packet_priv) references from the
code - and things stopped working, as with af_packet_priv==NULL ARP
replies are not delivered to the registered packet_type callback, as
these packets match skb->sk == pt->af_packet_priv for af_packet_priv == NULL.

I can of course set 'af_packet_priv' to some non-NULL value - but in such 
case I do not understand why you renamed it, if semantic is same as it was 
before.
                                                    Best regards,
                                                        Petr Vandrovec
                                                        vandrove@vc.cvut.cz
                                                        

Re: [PATCH?] Fix sniffing of ARP replies

[David S. Miller]

On Thu, 16 Oct 2003 12:07:23 +0200
"Petr Vandrovec" <VANDROVE@vc.cvut.cz> wrote:

> It behaves same way it behaved. But recently you renamed packet_type.data
> to packet_type.af_packet_priv, saying that af_packet_priv should be used
> only by AF_PACKET code, by nobody else. So I trusted you, removed
> packet_type.data (and packet_type.af_packet_priv) references from the
> code - and things stopped working, as with af_packet_priv==NULL ARP
> replies are not delivered to the registered packet_type callback, as
> these packets match skb->sk == pt->af_packet_priv for af_packet_priv == NULL.

vmware was trying to influence the behavior of input packet
delivery by setting ->data in a way which was never defined.
That facility existed and continues to exist for the sake
of AF_PACKET solely, what vmware was doing just happened to work.

Re: [PATCH?] Fix sniffing of ARP replies

[Joe Perches]

On Thu, 2003-10-16 at 15:21, David S. Miller wrote:
> On Thu, 16 Oct 2003 12:07:23 +0200
> "Petr Vandrovec" <VANDROVE@vc.cvut.cz> wrote:
> 
> > It behaves same way it behaved. But recently you renamed packet_type.data
> > to packet_type.af_packet_priv, saying that af_packet_priv should be used
> > only by AF_PACKET code, by nobody else. So I trusted you, removed
> > packet_type.data (and packet_type.af_packet_priv) references from the
> > code - and things stopped working, as with af_packet_priv==NULL ARP
> > replies are not delivered to the registered packet_type callback, as
> > these packets match skb->sk == pt->af_packet_priv for af_packet_priv == NULL.
> 
> vmware was trying to influence the behavior of input packet
> delivery by setting ->data in a way which was never defined.
> That facility existed and continues to exist for the sake
> of AF_PACKET solely, what vmware was doing just happened to work.

vmware wasn't just trying to influence, it used the implementation.

So tell me, where was this definition again?

If it was just how the code functioned, you have no basis for that
argument.  If you have commentary, design docs or even externally
published discussions, that's a different story.  I diddn't find any.

Code is code, readable or intentionally obscured.
Function is function.

Peter's code worked before, it stopped working
after the packet.h/PKT_CAN_SHARE_SKB removal.

You can change the implementation, but your argument is made hollow
by not having any docs.

Re: [PATCH?] Fix sniffing of ARP replies

[David S. Miller]

On Thu, 16 Oct 2003 15:41:00 -0700
Joe Perches <joe@perches.com> wrote:

> On Thu, 2003-10-16 at 15:21, David S. Miller wrote:
> vmware wasn't just trying to influence, it used the implementation.

It just so happened to be usable in this way.

Re: [PATCH?] Fix sniffing of ARP replies

[Petr Vandrovec]

On Thu, Oct 16, 2003 at 03:45:37PM -0700, David S. Miller wrote:
> On Thu, 16 Oct 2003 15:41:00 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > On Thu, 2003-10-16 at 15:21, David S. Miller wrote:
> > vmware wasn't just trying to influence, it used the implementation.
> 
> It just so happened to be usable in this way.
> 

I do not understand. At least on hardware and configurations I tested
code still works if I set af_packet_priv member to some non-NULL value.
It looked to me like that it is not intentional that ARP replies are not
delivered to the sniffer - from your reply it looks to me like that
only allowed user  of whole packet_type, not only af_packet_priv member,
is AF_PACKET, and that for correct functionality af_packet_priv member
must be non-NULL. Otherwise semantic of this call is ungrokable by
me (it is ungrokable anyway as vital information where checksum should
be placed is lost, but you already said that it is intentional that tcpdump
sees it wrong).

For now I understand that I should set af_packet_priv to non-NULL value.
						Best regards,
							Petr Vandrovec
							vandrove@vc.cvut.cz

Re: [PATCH?] Fix sniffing of ARP replies

[David S. Miller]

On Fri, 17 Oct 2003 01:48:47 +0200
Petr Vandrovec <vandrove@vc.cvut.cz> wrote:

> It looked to me like that it is not intentional that ARP replies are not
> delivered to the sniffer - from your reply it looks to me like that
> only allowed user  of whole packet_type, not only af_packet_priv member,
> is AF_PACKET, and that for correct functionality af_packet_priv member
> must be non-NULL.

I relooked at this, and you may have a point.  Let me study
this some more.

Thanks.