On Tue, Jan 23, 2018 at 08:19:54PM +0000, Lawrence Brakmo wrote:
> On 1/23/18, 11:50 AM, "Eric Dumazet" <eric.duma...@gmail.com> wrote:
>
> On Tue, 2018-01-23 at 14:39 -0500, Neal Cardwell wrote:
> > On Tue, Jan 23, 2018 at 2:20 PM, Lawrence Brakmo <bra...@fb.com> wrote:
> > > On 1/23/18, 9:30 AM, "Yuchung Cheng" <ych...@google.com> wrote:
> > >
> > > The original patch that changes TCP's congestion control via eBPF
> only
> > > re-initializes the new congestion control, if the BPF op is set
> to an
> > > (invalid) value beyond BPF_SOCK_OPS_NEEDS_ECN. Consequently TCP
> will
> > >
> > > What do you mean by “(invalid) value”?
> > >
> > > run the new congestion control from random states.
> > >
> > > This has always been possible with setsockopt, no?
> > >
> > > This patch fixes
> > > the issue by always re-init the congestion control like other
> means
> > > such as setsockopt and sysctl changes.
> > >
> > > The current code re-inits the congestion control when calling
> > > tcp_set_congestion_control except when it is called early on (i.e. op
> <=
> > > BPF_SOCK_OPS_NEEDS_ECN). In that case there is no need to
> re-initialize
> > > since it will be initialized later by TCP when the connection is
> established.
> > >
> > > Otherwise, if we always call tcp_reinit_congestion_control we would
> call
> > > tcp_cleanup_congestion_control when the congestion control has not
> been
> > > initialized yet.
> >
> > On Tue, Jan 23, 2018 at 2:20 PM, Lawrence Brakmo <bra...@fb.com> wrote:
> > > On 1/23/18, 9:30 AM, "Yuchung Cheng" <ych...@google.com> wrote:
> > >
> > > The original patch that changes TCP's congestion control via eBPF
> only
> > > re-initializes the new congestion control, if the BPF op is set
> to an
> > > (invalid) value beyond BPF_SOCK_OPS_NEEDS_ECN. Consequently TCP
> will
> > >
> > > What do you mean by “(invalid) value”?
> > >
> > > run the new congestion control from random states.
> > >
> > > This has always been possible with setsockopt, no?
> > >
> > > This patch fixes
> > > the issue by always re-init the congestion control like other
> means
> > > such as setsockopt and sysctl changes.
> > >
> > > The current code re-inits the congestion control when calling
> > > tcp_set_congestion_control except when it is called early on (i.e. op
> <=
> > > BPF_SOCK_OPS_NEEDS_ECN). In that case there is no need to
> re-initialize
> > > since it will be initialized later by TCP when the connection is
> established.
> > >
> > > Otherwise, if we always call tcp_reinit_congestion_control we would
> call
> > > tcp_cleanup_congestion_control when the congestion control has not
> been
> > > initialized yet.
> >
> > Interesting. So I wonder if the symptoms we were seeing were due to
> > kernels that did not yet have this fix:
> >
> > 27204aaa9dc6 ("tcp: uniform the set up of sockets after successful
> > connection):
> >
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=27204aaa9dc67b833b77179fdac556288ec3a4bf
> >
> > Before that fix, there could be TFO passive connections that at SYN
> time called:
> > tcp_init_congestion_control(child);
> > and then:
> > tcp_call_bpf(child, BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB);
> >
> > So that if the CC was switched in the
> > BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB handler then there would be no
> > init for the new module?
>
>
> Note that bpf_sock->op can be written by a malicious BPF filter.
>
> So, a malicious filter can switch from Cubic to BBR without re-init,
> and bad things can happen.
>
> I do not believe we should trust BPF here.
>
> Very good point Eric. One solution would be to make bpf_sock->op not
> writeable by
> the BPF program.
>
> Neal, you are correct that would have been a problem. I leave it up to you
> guys whether
> making bpf_sock->op not writeable by BPF program is enough or if it is safer
> to always
> re-init (as long as there is no problem calling
> tcp_cleanup_congestion_control when it
> has not been initialized.
I think allowing write into 'op' and 'replylong' was a mistake.
Only 'reply' field is used by tcp_call_bpf().
No reason to let programs write into other fields.
I think we have to fix it now before programs start to rely
on this undefined behavior.
