From: Jakub Kicinski > Sent: 28 July 2022 17:56 > > On Thu, 28 Jul 2022 09:31:04 -0700 Martin KaFai Lau wrote: > > If I understand the concern correctly, it may not be straight forward to > > grip the reason behind the testings at in_bpf() [ the in_task() and > > the current->bpf_ctx test ] ? Yes, it is a valid point. > > > > The optval.is_bpf bit can be directly traced back to the bpf_setsockopt > > helper and should be easier to reason about. > > I think we're saying the opposite thing. in_bpf() the context checking > function is fine. There is a clear parallel to in_task() and combined > with the capability check it should be pretty obvious what the code > is intending to achieve. > > sockptr_t::in_bpf which randomly implies that the lock is already held > will be hard to understand for anyone not intimately familiar with the > BPF code. Naming that bit is_locked seems much clearer. > > Which is what I believe Stan was proposing. Or make sk_setsockopt() be called after the integer value has been read and with the lock held. That saves any (horrid) conditional locking. Also sockptr_t should probably have been a structure with separate user and kernel address fields. Putting the length in there would (probably) save code. There then might be scope for pre-copying short user buffers into a kernel buffer while still allowing the requests that ignore the length copy directly from a user buffer. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
