Do not subtract the SLL[2]_HDR_LEN if the location is negative.

[fenner]

This fixes the offset issue I mention in tcpdump issue 480 about dumping on multiple interfaces - fix_program() modifies even negative offsets to take the SLL header into account, so the special SKF_AD_ values are not usable directly when using the SLL DLTs.

[mcr]

The commit message does not seem to reflect the committed code, which seems to be just addition of a cast.

[fenner]

The commit message does not seem to reflect the committed code, which seems to be just addition of a cast.

k is a bpf_u_int32.

(bpf_u_int32)-4068 is > SLL2_HDR_LEN.
(bpf_int32)-4068 is < SLL2_HDR_LEN.
Adding the cast prevents the > SLL2_HDR_LEN from triggering incorrectly on negative values.

[fenner]

By the way, for testing purposes I added

Index: libpcap-1.10.0-dev/pcap-linux.c
===================================================================
--- libpcap-1.10.0-dev.orig/pcap-linux.c
+++ libpcap-1.10.0-dev/pcap-linux.c
@@ -3071,6 +3071,10 @@ pcap_setfilter_linux_common(pcap_t *hand
                         * work in the kernel.
                         */
                        can_filter_in_kernel = 0;
+                        if (1) {
+                                printf( "Filtering in userland:\n" );
+                                bpf_dump(&fcode, 1);
+                        }
                        break;
 
                case 1:
@@ -3078,6 +3082,10 @@ pcap_setfilter_linux_common(pcap_t *hand
                         * We have a filter that'll work in the kernel.
                         */
                        can_filter_in_kernel = 1;
+                        if (1) {
+                                printf( "Installing in kernel:\n" );
+                                bpf_dump(&fcode, 1);
+                        }
                        break;
                }
        }

and the dumped code for ether[-4068:4] was clearly modified before adding the cast, and not modified after adding the cast.

[guyharris]

This fixes the offset issue I mention in #480 - fix_program() modifies even negative offsets to take the SLL header into account, so the special SKF_AD_ values are not usable directly when using the SLL DLTs.

Presumably you mean tcpdump issue 480, i.e. the-tcpdump-group/tcpdump#480, not just #480, which refers to libpcap issue 480.

[guyharris]

This fixes the offset issue I mention in #480 - fix_program() modifies even negative offsets to take the SLL header into account, so the special SKF_AD_ values are not usable directly when using the SLL DLTs.

Presumably you mean tcpdump issue 480, i.e. the-tcpdump-group/tcpdump#480, not just #480, which refers to libpcap issue 480.

And that'd be this comment.

[fenner]

This fixes the offset issue I mention in #480 - fix_program() modifies even negative offsets to take the SLL header into account, so the special SKF_AD_ values are not usable directly when using the SLL DLTs.

Presumably you mean tcpdump issue 480, i.e. the-tcpdump-group/tcpdump#480, not just #480, which refers to libpcap issue 480.

And that'd be this comment.

Thanks, Guy, I wasn't being careful about what I wrote. I've updated the description.

[fenner]

I updated the code to match Guy's suggestion, and the test is:

//src/libpcap @fenner-t493-2.sjc% tcpdump -i any 'ether[-4096] = 0'
Installing in kernel:
(000) ldb      [-4096]
(001) jeq      #0x0             jt 2    jf 3
(002) ret      #262144
(003) ret      #0
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on any, link-type LINUX_SLL (Linux cooked v1), snapshot length 262144 bytes
^C
0 packets captured

vs.

//src/libpcap @fenner-t493-2.sjc% tcpdump -i any 'ether[-4097] = 0'
Installing in kernel:
(000) ldb      [-4113]
(001) jeq      #0x0             jt 2    jf 3
(002) ret      #262144
(003) ret      #0
tcpdump: verbose output suppressed, use -v or -vv for full protocol decode
listening on any, link-type LINUX_SLL (Linux cooked v1), snapshot length 262144 bytes
^C
0 packets captured

(The filter debug output is identical with -y linux_sll2)

[guyharris]

So the question is whether an offset of -4097 should be interpreted as SKF_AD_OFF + (-1) or as 4294963199.

bpf_check_classic() in Linux net/core/filter.c is the routine that checks "classic" BPF (as opposed to eBPF) code for validity. For "load absolute" instructions, the test is

		anc_found = false;
		if (bpf_anc_helper(ftest) & BPF_ANC)
			anc_found = true;
		/* Ancillary operation unknown or unsupported */
		if (anc_found == false && ftest->k >= SKF_AD_OFF)
			return -EINVAL;

bpf_anc_helper() is an inline in include/linux/filter.h; if it's handed an absolute load instruction, it returns BPF_ANC | SKF_AD_##CODE for all offsets of the form SKF_AD_OFF + SKF_AD_##CODE, for SKF_AD_##CODE being one of the known codes for ancillary data. Otherwise - i.e., if the offset isn't one of those, or the instruction isn't an absolute load, it returns the opcode. It appears to fail with a (non-panic?) kernel bug if the opcode has the BPF_ANC bit set, so presumably it returns a value with BPF_ANC set only if it's a load from one of those known ancillary data offsets.

So it looks as if the intent is that the only ancillary data offsets that are valid are the ones of the form SKF_AD_OFF + xxx, for xxx being one of

#define SKF_AD_PROTOCOL 0
#define SKF_AD_PKTTYPE 	4
#define SKF_AD_IFINDEX 	8
#define SKF_AD_NLATTR	12
#define SKF_AD_NLATTR_NEST	16
#define SKF_AD_MARK 	20
#define SKF_AD_QUEUE	24
#define SKF_AD_HATYPE	28
#define SKF_AD_RXHASH	32
#define SKF_AD_CPU	36
#define SKF_AD_ALU_XOR_X	40
#define SKF_AD_VLAN_TAG	44
#define SKF_AD_VLAN_TAG_PRESENT 48
#define SKF_AD_PAY_OFFSET	52
#define SKF_AD_RANDOM	56
#define SKF_AD_VLAN_TPID	60

The comment before that is

/* RATIONALE. Negative offsets are invalid in BPF.
   We use them to reference ancillary data.
   Unlike introduction new instructions, it does not break
   existing compilers/optimizers.
 */

although the structure for the instruction is

struct sock_filter {	/* Filter block */
	__u16	code;   /* Actual filter code */
	__u8	jt;	/* Jump true */
	__u8	jf;	/* Jump false */
	__u32	k;      /* Generic multiuse field */
};

so the offset, being unsigned, is never negative. It's almost certainly past the end of the packet, and they may, in fact, limit offsets somewhere to be < 2^31-1.

They also define

#define SKF_AD_MAX	64

I don't know whether "MAX" means "this is the largest valid ancillary data offset" or "this is just past the largest valid ancillary data offset" - if it's the former, they're presumably reserving it or something.

So should we 1) treat all offsets with the uppermost bit set as being special, 2) treat all offsets > (bpf_u_int32) SKF_AD_OFF as being special, or 3) treat all offsets in the range from (bpf_u_int32) SKF_AD_OFF to just before (bpf_u_int32) (SKF_AD_OFF + SKF_AD_MAX) as being special?

[fenner]

So should we 1) treat all offsets with the uppermost bit set as being special, 2) treat all offsets > (bpf_u_int32) SKF_AD_OFF as being special, or 3) treat all offsets in the range from (bpf_u_int32) SKF_AD_OFF to just before (bpf_u_int32) (SKF_AD_OFF + SKF_AD_MAX) as being special?

Your (1) is more like my original code's behavior. (2) is what my current code does. I'm fine with either.

(3) is funny because what we do is subtract from k, so SKF_AD_OFF + SKF_AD_MAX + 4 would end up less than SKF_AD_OFF + SKF_AD_MAX after subtraction (meaning, it has one of the special SKF_AD_foo behaviors even though it started out outside the special range). This is sufficiently bizarre that I think it's worth avoiding.

[fenner]

I've been rebasing to make it easier to accept. Would you like me to change the behavior to treat all negative offsets as special (your (1)), or leave the current behavior (your (2))?

[fenner]

I've been rebasing to make it easier to accept. Would you like me to change the behavior to treat all negative offsets as special (your (1)), or leave the current behavior (your (2))?