On Thu, Sep 21, 2023 at 8:26 AM Paolo Abeni pabeni@redhat.com wrote:
On Thu, 2023-09-21 at 09:30 -0400, Willem de Bruijn wrote:
On Thu, Sep 21, 2023 at 4:35 AM Paolo Abeni pabeni@redhat.com wrote:
On Wed, 2023-09-20 at 09:30 -0400, Willem de Bruijn wrote:
Jordan Rife wrote:
Similar to the change in commit 0bdf399342c5("net: Avoid address overwrite in kernel_connect"), BPF hooks run on bind may rewrite the address passed to kernel_bind(). This change
- Makes a copy of the bind address in kernel_bind() to insulate callers.
- Replaces direct calls to sock->ops->bind() with kernel_bind()
Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/ Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind") Cc: stable@vger.kernel.org Signed-off-by: Jordan Rife jrife@google.com
Reviewed-by: Willem de Bruijn willemb@google.com
I fear this is going to cause a few conflicts with other trees. We can still take it, but at very least we will need some acks from the relevant maintainers.
I *think* it would be easier split this and patch 1/3 in individual patches targeting the different trees, hopefully not many additional patches will be required. What do you think?
Roughly how many patches would result from this one patch. From the stat line I count { block/drbd, char/agp, infiniband, isdn, fs/dlm, fs/ocfs2, fs/smb, netfilter, rds }. That's worst case nine callers plus the core patch to net/socket.c?
I think there should not be problems taking directly changes for rds and nf/ipvs.
Additionally, I think the non network changes could consolidate the bind and connect changes in a single patch.
It should be 7 not-network patches overall.
If logistically simpler and you prefer the approach, we can also revisit Jordan's original approach, which embedded the memcpy inside the BPF branches.
That has the slight benefit to in-kernel callers that it limits the cost of the memcpy to cgroup_bpf_enabled. But adds a superfluous second copy to the more common userspace callers, again at least only if cgroup_bpf_enabled.
If so, it should at least move the whole logic around those BPF hooks into helper functions.
IMHO the approach implemented here is preferable, I suggest going forward with it.
Thanks,
Paolo
Additionally, I think the non network changes could consolidate the bind and connect changes in a single patch.
It should be 7 not-network patches overall.
I'm fine with this. If there are no objections, I can drop the non-net changes in this patch series and send out several kernel_connect/kernel_bind patches to the appropriate trees as a follow up. Shall we wait to hear back from the maintainers or just go ahead with this plan?
-Jordan