RE: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
On Monday 2015-11-23 18:35, David Laight wrote: >From: Florian Westphal >> Sent: 21 November 2015 16:56 >> > +struct xt_cgroup_info_v1 { >> > + charpath[PATH_MAX]; >> > + __u32 classid; >> > + >> > + /* kernel internal data */ >> > + void*priv __attribute__((aligned(8))); >> > +}; >> >> Ahem. Am I reading this right? This struct is > 4k in size? >> If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? > >I've not looked at the use, but could you put 'char path[];' >as the last member an require any allocations to be long enough >to contain the actual path? Oh, smart :) Yeah, ebt_among does something like that. (.matchsize = -1, hint) Except that the "priv" pointer seems to be ruining the fun here - kernel vars have to be last, which collides with the requirements for []-type members. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
From: Florian Westphal > Sent: 21 November 2015 16:56 > > +struct xt_cgroup_info_v1 { > > + __u8has_path; > > + __u8has_classid; > > + __u8invert_path; > > + __u8invert_classid; > > + charpath[PATH_MAX]; > > + __u32 classid; > > + > > + /* kernel internal data */ > > + void*priv __attribute__((aligned(8))); > > +}; > > Ahem. Am I reading this right? This struct is > 4k in size? > If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? I've not looked at the use, but could you put 'char path[];' as the last member an require any allocations to be long enough to contain the actual path? David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Hello, On Mon, Nov 23, 2015 at 01:43:01PM +0100, Daniel Wagner wrote: > Hi Tejun, > > On 11/21/2015 05:14 PM, Tejun Heo wrote:> +static int > > cgroup_mt_check_v1(const struct xt_mtchk_param *par) > > +{ > > + struct xt_cgroup_info_v1 *info = par->matchinfo; > > + struct cgroup *cgrp; > > + > > + if ((info->invert_path & ~1) || (info->invert_classid & ~1)) > > + return -EINVAL; > > The checks below use pr_info() in case the configuration is not valid. > Is this missing here on purpose? It's mostly copied from v0 function but I think it makes sense. The other errors can be caused by incorrect user input but the above one can't happen unless iptables extension itself is broken. > I have tested it slightly and it seems to work (also on an older > kernel). I don't know if that qualifies it for a Tested-by but at least > Acked-by should do the trick: Will answer that there. > Tested-by: Daniel Wagner > Acked-by: Daniel Wagner Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Hello, Daniel. On Mon, Nov 23, 2015 at 02:43:12PM +0100, Daniel Borkmann wrote: ... > Haven't looked deeply into kernfs, but if it's possible to get the object > from the struct file eventually, you could let iptables frontend open that > path and just pass the fd down. Would be sizeof(int) vs PATH_MAX then, i.e. > when you have a large number of rules to load. That works in one direction but not in the other. ie. You can tell the kernel the path that way but can't retrieve. If using path string is unacceptable, the best alternative would be an inode number rather than a fd; however, using an inode number would be quite cumbersome and painful too. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
On 11/23/2015 02:43 PM, Daniel Borkmann wrote: On 11/21/2015 07:54 PM, Florian Westphal wrote: Tejun Heo wrote: On Sat, Nov 21, 2015 at 05:56:06PM +0100, Florian Westphal wrote: +struct xt_cgroup_info_v1 { +__u8has_path; +__u8has_classid; +__u8invert_path; +__u8invert_classid; +charpath[PATH_MAX]; +__u32classid; + +/* kernel internal data */ +void*priv __attribute__((aligned(8))); +}; Ahem. Am I reading this right? This struct is > 4k in size? If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? Hmmm... yeap but would this be an acutual problem? Since rule blob can be allocated via vmalloc i guess "no", its not really a problem unless someone needs realy insane amount of such rules. I don't have any better suggestion, so I guess its necessary evil. The only other question I have is wheter PATH_MAX might be a possible ABI breaker in future. It would have to be guaranteed that this is the same size forever, else you'd get strange errors on rule insertion if the sizes of the kernel and userspace version differs. Haven't looked deeply into kernfs, but if it's possible to get the object from the struct file eventually, you could let iptables frontend open that path and just pass the fd down. Would be sizeof(int) vs PATH_MAX then, i.e. when you have a large number of rules to load. ( ... but with the downside that things like save/restore wouldn't work. ) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
On 11/21/2015 07:54 PM, Florian Westphal wrote: Tejun Heo wrote: On Sat, Nov 21, 2015 at 05:56:06PM +0100, Florian Westphal wrote: +struct xt_cgroup_info_v1 { + __u8has_path; + __u8has_classid; + __u8invert_path; + __u8invert_classid; + charpath[PATH_MAX]; + __u32 classid; + + /* kernel internal data */ + void*priv __attribute__((aligned(8))); +}; Ahem. Am I reading this right? This struct is > 4k in size? If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? Hmmm... yeap but would this be an acutual problem? Since rule blob can be allocated via vmalloc i guess "no", its not really a problem unless someone needs realy insane amount of such rules. I don't have any better suggestion, so I guess its necessary evil. The only other question I have is wheter PATH_MAX might be a possible ABI breaker in future. It would have to be guaranteed that this is the same size forever, else you'd get strange errors on rule insertion if the sizes of the kernel and userspace version differs. Haven't looked deeply into kernfs, but if it's possible to get the object from the struct file eventually, you could let iptables frontend open that path and just pass the fd down. Would be sizeof(int) vs PATH_MAX then, i.e. when you have a large number of rules to load. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Hi Tejun, On 11/21/2015 05:14 PM, Tejun Heo wrote:> +static int > cgroup_mt_check_v1(const struct xt_mtchk_param *par) > +{ > + struct xt_cgroup_info_v1 *info = par->matchinfo; > + struct cgroup *cgrp; > + > + if ((info->invert_path & ~1) || (info->invert_classid & ~1)) > + return -EINVAL; The checks below use pr_info() in case the configuration is not valid. Is this missing here on purpose? I have tested it slightly and it seems to work (also on an older kernel). I don't know if that qualifies it for a Tested-by but at least Acked-by should do the trick: Tested-by: Daniel Wagner Acked-by: Daniel Wagner cheers, daniel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
On Saturday 2015-11-21 19:54, Florian Westphal wrote: > >The only other question I have is wheter PATH_MAX might be a possible >ABI breaker in future. It would have to be guaranteed that this is the >same size forever, else you'd get strange errors on rule insertion if >the sizes of the kernel and userspace version differs. > The same goes for IFNAMSIZ. But, so far, nobody changed it in the kernel, even though there are voices that 15 characters + '\0' were a tight choice. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Tejun Heo wrote: > On Sat, Nov 21, 2015 at 05:56:06PM +0100, Florian Westphal wrote: > > > +struct xt_cgroup_info_v1 { > > > + __u8has_path; > > > + __u8has_classid; > > > + __u8invert_path; > > > + __u8invert_classid; > > > + charpath[PATH_MAX]; > > > + __u32 classid; > > > + > > > + /* kernel internal data */ > > > + void*priv __attribute__((aligned(8))); > > > +}; > > > > Ahem. Am I reading this right? This struct is > 4k in size? > > If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? > > Hmmm... yeap but would this be an acutual problem? Since rule blob can be allocated via vmalloc i guess "no", its not really a problem unless someone needs realy insane amount of such rules. I don't have any better suggestion, so I guess its necessary evil. The only other question I have is wheter PATH_MAX might be a possible ABI breaker in future. It would have to be guaranteed that this is the same size forever, else you'd get strange errors on rule insertion if the sizes of the kernel and userspace version differs. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Hello, On Sat, Nov 21, 2015 at 05:56:06PM +0100, Florian Westphal wrote: > > +struct xt_cgroup_info_v1 { > > + __u8has_path; > > + __u8has_classid; > > + __u8invert_path; > > + __u8invert_classid; > > + charpath[PATH_MAX]; > > + __u32 classid; > > + > > + /* kernel internal data */ > > + void*priv __attribute__((aligned(8))); > > +}; > > Ahem. Am I reading this right? This struct is > 4k in size? > If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? Hmmm... yeap but would this be an acutual problem? We can try to make it shorter but idk it ultimately is a path. Another solution would be trying to pass inode around but that is problematic with showing and printing rules as the only way to reverse-map inode to path is walking the tree and the cgroup may already be gone at that point. While >4k struct isn't pretty, this looks like the path of least resistance. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 9/9] netfilter: implement xt_cgroup cgroup2 path match
Tejun Heo wrote: > This patch implements xt_cgroup path match which matches cgroup2 > membership of the associated socket. The match is recursive and > invertible. > > For rationales on introducing another cgroup based match, please refer > to a preceding commit "sock, cgroup: add sock->sk_cgroup". > > v3: Folded into xt_cgroup as a new revision interface as suggested by > Pablo. > > v2: Included linux/limits.h from xt_cgroup2.h for PATH_MAX. Added > explicit alignment to the priv field. Both suggested by Jan. > > Signed-off-by: Tejun Heo > Cc: Daniel Borkmann > Cc: Daniel Wagner > CC: Neil Horman > Cc: Jan Engelhardt > Cc: Pablo Neira Ayuso > --- > include/uapi/linux/netfilter/xt_cgroup.h | 13 ++ > net/netfilter/xt_cgroup.c| 69 > > 2 files changed, 82 insertions(+) > > diff --git a/include/uapi/linux/netfilter/xt_cgroup.h > b/include/uapi/linux/netfilter/xt_cgroup.h > index 577c9e0..1e4b37b 100644 > --- a/include/uapi/linux/netfilter/xt_cgroup.h > +++ b/include/uapi/linux/netfilter/xt_cgroup.h > @@ -2,10 +2,23 @@ > #define _UAPI_XT_CGROUP_H > > #include > +#include > > struct xt_cgroup_info_v0 { > __u32 id; > __u32 invert; > }; > > +struct xt_cgroup_info_v1 { > + __u8has_path; > + __u8has_classid; > + __u8invert_path; > + __u8invert_classid; > + charpath[PATH_MAX]; > + __u32 classid; > + > + /* kernel internal data */ > + void*priv __attribute__((aligned(8))); > +}; Ahem. Am I reading this right? This struct is > 4k in size? If so -- Ugh. Does sizeof(path) really have to be PATH_MAX? Thanks! -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html