On Sun, Aug 5, 2018 at 4:39 PM, Dominique Martinet
<asmad...@codewreck.org> wrote:
> 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;
> + }
> +
Seems okay to me for a fix. Looks like strp.offset is only set in one
place and read in one place. With this pull maybe that just can go
away?
Tom
> 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
