On Tue, Jan 17, 2023 at 2:51 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > On Mon, Jan 16, 2023 at 2:35 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > On Mon, Jan 16, 2023 at 1:13 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > On Mon, Jan 16, 2023 at 12:37 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > > On Mon, Jan 16, 2023 at 11:46 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > > On Sat, Jan 14, 2023 at 12:54 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > > > > > On Sat, Jan 14, 2023 at 10:39 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > > > > > > > On Fri, Jan 13, 2023 at 10:31 PM Xin Long <lucien.xin@xxxxxxxxx> wrote: > > ... > > > > > > We can't skip the CIPSO labeling as that would be the network packet > > > > > equivalent of not assigning a owner/group/mode to a file on the > > > > > filesystem, which is a Very Bad Thing :) > > > > > > > > > > I spent a little bit of time this morning looking at the problem and I > > > > > think the right approach is two-fold: first introduce a simple check > > > > > in cipso_v4_skbuff_setattr() which returns -E2BIG if the packet length > > > > > grows beyond 65535. It's rather crude, but it's a tiny patch and > > > > > should at least ensure that the upper layers (NetLabel and SELinux) > > > > > don't send the packet with a bogus length field; it will result in > > > > > packet drops, but honestly that seems preferable to a mangled packet > > > > > which will likely be dropped at some point in the network anyway. > > > > > > > > > > diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c > > > > > index 6cd3b6c559f0..f19c9beda745 100644 > > > > > --- a/net/ipv4/cipso_ipv4.c > > > > > +++ b/net/ipv4/cipso_ipv4.c > > > > > @@ -2183,8 +2183,10 @@ int cipso_v4_skbuff_setattr(struct sk_buff *skb, > > > > > * that the security label is applied to the packet - we do the same > > > > > * thing when using the socket options and it hasn't caused a problem, > > > > > * if we need to we can always revisit this choice later */ > > > > > - > > > > > len_delta = opt_len - opt->optlen; > > > > > + if ((skb->len + len_delta) > 65535) > > > > > + return -E2BIG; > > > > > + > > > > > > > > Right, looks crude. :-) > > > > > > Yes, but what else can we do? There is fragmentation, but that is > > > rather ugly and we would still need a solution for when the don't > > > fragment bit is set. I'm open to suggestions. > > > > looking at ovs_dp_upcall(), for GSO/GRO packets it goes to > > queue_gso_packets() where it calls __skb_gso_segment() > > to segment it into small segs/skbs, then process these segs instead. > > > > I'm thinking you can try to do the same in cipso_v4_skbuff_setattr(), > > and I don't think 64K non-GSO packets exist in the user environment, > > so taking care of GSO packets should be enough. > > Thanks, I'll take a look. Unfortunately I don't think the ovs_dp_upcall() approach will work as that is an endpoint in the kernel which sends the GSO'd packet up to userspace in segements. In the case of cipso_v4_skbuff_setattr() we are setting an IPv4 option on a packet in either the NF_INET_LOCAL_OUT or NF_INET_FORWARD output path. I believe we can resolve the LOCAL_OUT case with the padding approach I mentioned previously, but the FORWARD path remains a challenge; I simply don't see a way to handle growing the packet beyond 64k in the forward path. I'm also realizing that we should be sending a ICMP_FRAG_NEEDED in the forward case when we have to drop the packet due to size issues, as the normal MTU/size check happens prior to the NF_INET_FORWARD hooks (and hence cipso_v4_skbuff_setattr()). > > > It seems like there is still ongoing discussion about even enabling > > > BIG TCP for IPv4, however for this discussion let's assume that BIG > > > TCP is merged for IPv4. > > > > > > We really should have a solution that allows CIPSO for both normal and > > > BIG TCP, if we don't we force distros and admins to choose between the > > > two and that isn't good. We should do better. If skb->len > 64k in > > > the case of BIG TCP, how is the packet eventually divided/fragmented > > > in such a way that the total length field in the IPv4 header doesn't > > > overflow? Or is that simply handled at the driver/device layer and we > > > simply set skb->len to whatever the size is, regardless of the 16-bit > > > > Yes, for BIG TCP, 16-bit length is set to 0, and it just uses skb->len > > as the IP packet length. > > In the BIG TCP case, when is the IPv4 header zero'd out? Currently > cipso_v4_skbuff_setattr() is called in the NF_INET_LOCAL_OUT and > NF_INET_FORWARD chains, is there an easy way to distinguish between a > traditional segmentation offload mechanism, e.g. GSO, and BIG TCP? If > BIG TCP allows for arbitrarily large packets we can just grow the > skb->len value as needed and leave the total length field in the IPv4 > header untouched/zero, but we would need to be able to distinguish > between a segmentation offload and BIG TCP. Keeping the above questions as they still apply, rather I could still use some help understanding what a BIG TCP packet would look like during LOCAL_OUT and FORWARD. > > > In the GRO case, is it safe to grow the packet such that skb->len is > > > greater than 64k? I presume that the device/driver is going to split > > > the packet anyway and populate the IPv4 total length fields in the > > > header anyway, right? If we can't grow the packet beyond 64k, is > > > there some way to signal to the driver/device at runtime that the > > > largest packet we can process is 64k minus 40 bytes (for the IPv4 > > > options)? > > > > at runtime, not as far as I know. > > It's a field of the network device that can be modified by: > > # ip link set dev eth0 gro_max_size $MAX_SIZE gso_max_size $MAX_SIZE > > I need to look at the OVS case above, but one possibility would be to > have the kernel adjust the GSO size down by 40 bytes when > CONFIG_NETLABEL is enabled, but that isn't a great option, and not > something I consider a first (or second) choice. Looking more at the GSO related code, this isn't likely to work. -- paul-moore.com
