Kumar Kartikeya Dwivedi wrote:
> Hello John,
>
> For background, I have a sk_skb program where for certain flows, a
> subset of request handling logic (on Rx) that is typically done in
> user space is handed over to a BPF program for doing it in-kernel.
>
> Since the protocol is over TCP, I'm using sk_skb programs to handle
> these requests and generate responses from within the kernel. The user
> space application will decide which flow should be added to the
> sockmap on socket accept and then let it be handled in the kernel for
> some of the request types.
>
> However, when generating replies and trimming away extra bytes in the
> packet by using bpf_skb_adjust_room, I see packet drops because the
> strp_msg->full_len is not updated unless tls_sw_has_ctx_rx is true for
> the socket. Removing that condition makes everything work, but I am
> not sure that is the right fix.
>
> My understanding so far is that in sk_psock_backlog,
> sk_psock_handle_skb returns an error since the stm->full_len is not
> correct (and passed into it as len), and SK_PSOCK_TX_ENABLED gets
> cleared due to this error. Then, on the redirect side
> (sk_psock_skb_redirect), it does sock_drop because the test for the
> SK_PSOCK_TX_ENABLED flag fails, which does kfree_skb.
>
> I have attached a patch with a selftest (skb_adjust_room) that you can
> run to verify the problem. It should basically hang on the second read
> as the packet would be dropped by the kernel. The first read goes
> through fine. Applying the diff below allows the selftest to pass.
>
> I am not familiar with the code to know whether the fix below is
> correct, but it seems to resolve the problem for me (and I carried it
> for a while and ran my stuff on it until I got around to reporting
> this now).
>
> In case gmail screws up the formatting, I'm just removing the
> tls_sw_has_ctx_rx(skb->sk) conditional on the strp_msg->full_len
> update in sk_skb_adjust_room.
I think your analysis is correct and patch looks good. I'll study
a bit more tomorrow. It looks like I just messed it up there and
didn't consider the case of redirect in non-TLS case.
Gmail did fine pushing code through.
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 524adf1fa6d0..e3009d0f3352 100644
>
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3622,11 +3622,7 @@ BPF_CALL_4(sk_skb_adjust_room, struct sk_buff
> *, skb, s32, len_diff,
>
> return -ENOMEM;
>
> __skb_pull(skb, len_diff_abs);
>
> }
>
> - if (tls_sw_has_ctx_rx(skb->sk)) {
> - struct strp_msg *rxm = strp_msg(skb);
> -
> - rxm->full_len += len_diff;
> - }
> + strp_msg(skb)->full_len += len_diff;
> return ret;
> }
>
> --
>
> Thanks!
