Use a 16-bit comparison for "pppoes NUM".

gen_pppoes() compares only the low 16 bits of a [32-bit] word, which
does not entirely match the problem space.  Eliminate the bitwise AND
instruction by comparing a [16-bit] half-word with the same 16 bits,
this produces simpler bytecode (thus the corresponding accept test
changes) with an equivalent behaviour (thus no apply tests change).
While at it, add a comment with the packet header diagram and use a
standard named constant for the maximum session ID value.

diff --git a/gencode.c b/gencode.c
index 1dbff96dca5e68a1e087b645470cee88b28eb4eb..5c94e60d45732d46d311a3a84023d6e9a44d8195 100644 (file)
--- a/gencode.c
+++ b/gencode.c
@@ -9550,6 +9550,19 @@ gen_pppoed(compiler_state_t *cstate)
        return gen_linktype(cstate, ETHERTYPE_PPPOED);
 }
 
+/*
+ * RFC 2516 Section 4:
+ *
+ * The Ethernet payload for PPPoE is as follows:
+ *
+ *                      1                   2                   3
+ *  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |  VER  | TYPE  |      CODE     |          SESSION_ID           |
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ * |            LENGTH             |           payload             ~
+ * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+ */
 struct block *
 gen_pppoes(compiler_state_t *cstate, bpf_u_int32 sess_num, int has_sess_num)
 {
@@ -9569,11 +9582,11 @@ gen_pppoes(compiler_state_t *cstate, bpf_u_int32 sess_num, int has_sess_num)
 
        /* If a specific session is requested, check PPPoE session id */
        if (has_sess_num) {
-               if (sess_num > 0x0000ffff) {
+               if (sess_num > UINT16_MAX) {
                        bpf_error(cstate, "PPPoE session number %u greater than maximum %u",
-                           sess_num, 0x0000ffff);
+                           sess_num, UINT16_MAX);
                }
-               b1 = gen_mcmp(cstate, OR_LINKPL, 0, BPF_W, sess_num, 0x0000ffff);
+               b1 = gen_cmp(cstate, OR_LINKPL, 2, BPF_H, sess_num);
                gen_and(b0, b1);
                b0 = b1;
        }

diff --git a/testprogs/TESTrun b/testprogs/TESTrun
index cdcdbb23d1d90179c1c059aae4ecb105cfbe0cc1..a79a18fe5a00c6e21801a29522dd37f9c4126701 100755 (executable)
--- a/testprogs/TESTrun
+++ b/testprogs/TESTrun
@@ -3316,12 +3316,11 @@ my %accept_blocks = (
                expr => 'pppoes 1234',
                unopt => <<~'EOF',
                        (000) ldh      [12]
-                       (001) jeq      #0x8864          jt 2    jf 6
-                       (002) ld       [14]
-                       (003) and      #0xffff
-                       (004) jeq      #0x4d2           jt 5    jf 6
-                       (005) ret      #200
-                       (006) ret      #0
+                       (001) jeq      #0x8864          jt 2    jf 5
+                       (002) ldh      [16]
+                       (003) jeq      #0x4d2           jt 4    jf 5
+                       (004) ret      #200
+                       (005) ret      #0
                        EOF
        }, # pppoes_unary