Re: KCM - recvmsg() mangles packets?

[Dominique Martinet <asmadeus@codewreck.org>]

Dominique Martinet wrote on Sun, Aug 05, 2018:
> It's getting late but I'll try adding a pskb_pull in there tomorrow, it
> would be better to make the bpf program start with an offset but I don't
> think that'll be easy to change...

I can confirm the following patch fixes the issue for me:
-----8<---------------------
diff --git a/net/strparser/strparser.c b/net/strparser/strparser.c
index 625acb27efcc..348ff5945591 100644
--- a/net/strparser/strparser.c
+++ b/net/strparser/strparser.c
@@ -222,6 +222,16 @@ static int __strp_recv(read_descriptor_t *desc, struct sk_buff *orig_skb,
 		if (!stm->strp.full_len) {
 			ssize_t len;
 
+			/* Can only parse if there is no offset */
+			if (unlikely(stm->strp.offset)) {
+				if (!pskb_pull(skb, stm->strp.offset)) {
+					STRP_STATS_INCR(strp->stats.mem_fail);
+					strp_parser_err(strp, -ENOMEM, desc);
+					break;
+				}
+				stm->strp.offset = 0;
+			}
+
 			len = (*strp->cb.parse_msg)(strp, head);
 
 			if (!len) {
----------------8<----------------------

Now, I was looking at other users of strparser (I see sockmap, kcm and
tls) and it looks like sockmap does not handle offsets either but tls
does by using skb_copy_bits -- they're copying the tls header to a
buffer on the stack.

kcm cannot do that because we do not know how much data the user expects
to read, and I'm not comfortable doing pskb_pull in the kcm callback
either, but the cost of this pull is probably non-negligible if some
user can make do without it...

On the other hand, I do not see how to make the bpf program handle an
offset in the skb as that offset is strparser-specific.

Maybe add a flag in the cb that specifies wether the callback allows
non-zero offset?


I'll let you see if you can reproduce this and will wait for advices on
how to solve this properly so we can work on a proper fix.


Thanks,
-- 
Dominique