| Message ID | 20170917112031.8644-2-shmulik@nsof.io |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | xt_bpf: fix handling of pinned objects | expand |
Hi Rafael, On Sun, Sep 17, 2017 at 02:20:30PM +0300, Shmulik Ladkani wrote: > From: Rafael Buchbinder <rafi@rbk.ms> > > From: Rafael Buchbinder <rafi@rbk.ms> > > This commit introduces a framework to fixup match info, > which may be required by an extension. > > Signed-off-by: Rafael Buchbinder <rafi@rbk.ms> > Signed-off-by: Shmulik Ladkani <shmulik@nsof.io> > --- > include/xtables.h | 3 +++ > iptables/ip6tables.c | 35 +++++++++++++++++++++++++++++++++++ > iptables/iptables.c | 34 ++++++++++++++++++++++++++++++++++ > 3 files changed, 72 insertions(+) > > diff --git a/include/xtables.h b/include/xtables.h > index e9bc3b7d..687cfe9f 100644 > --- a/include/xtables.h > +++ b/include/xtables.h > @@ -273,6 +273,9 @@ struct xtables_match { > /* ip is struct ipt_ip * for example */ > void (*save)(const void *ip, const struct xt_entry_match *match); > > + /* Fixes the match info after init. */ > + void (*tc_init_fixup)(struct xt_entry_match *match); If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo, On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > + /* Fixes the match info after init. */ > > + void (*tc_init_fixup)(struct xt_entry_match *match); > > If this is only broken from tc ipt actions, could you fix this from > iproute2/tc instead? No, this is not iproute2/tc specfic. We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call. If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'. This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel. The problem lies in the xt_bpf_info_v1 ABI. See: https://marc.info/?l=netfilter-devel&m=150530909630143&w=2 Regards, Shmulik -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote: > Hi Pablo, > > On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > > > > + /* Fixes the match info after init. */ > > > + void (*tc_init_fixup)(struct xt_entry_match *match); > > > > If this is only broken from tc ipt actions, could you fix this from > > iproute2/tc instead? > > No, this is not iproute2/tc specfic. OK. > We named it 'tc_init_fixup' as it occurs just after the TC_INIT > (iptc_init/ip6tc_init) call. > If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' > or 'iptc_init_fixup'. > > This must occur after every load of entries, as the xt_bpf match needs > a fixup once read from kernel. > > The problem lies in the xt_bpf_info_v1 ABI. > See: > https://marc.info/?l=netfilter-devel&m=150530909630143&w=2 I see, can we get a v2 ABI that fixes this? Given this was included not long time ago, we can quickly deprecate this without this custom hook to address this. We can include this in the next iptables release in the next weeks. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote: >> Hi Pablo, >> >> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> >> > > >> > > + /* Fixes the match info after init. */ >> > > + void (*tc_init_fixup)(struct xt_entry_match *match); >> > >> > If this is only broken from tc ipt actions, could you fix this from >> > iproute2/tc instead? >> >> No, this is not iproute2/tc specfic. > > OK. > >> We named it 'tc_init_fixup' as it occurs just after the TC_INIT >> (iptc_init/ip6tc_init) call. >> If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' >> or 'iptc_init_fixup'. >> >> This must occur after every load of entries, as the xt_bpf match needs >> a fixup once read from kernel. >> >> The problem lies in the xt_bpf_info_v1 ABI. >> See: >> https://marc.info/?l=netfilter-devel&m=150530909630143&w=2 > > I see, can we get a v2 ABI that fixes this? Given this was included > not long time ago, we can quickly deprecate this without this custom > hook to address this. We can perhaps change the kernel module to ignore .fd and do a path lookup for .path directly inside the kernel. That would not require a v2, even. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 18, 2017 at 01:50:32PM -0400, Willem de Bruijn wrote: > On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote: > >> Hi Pablo, > >> > >> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > >> > >> > > > >> > > + /* Fixes the match info after init. */ > >> > > + void (*tc_init_fixup)(struct xt_entry_match *match); > >> > > >> > If this is only broken from tc ipt actions, could you fix this from > >> > iproute2/tc instead? > >> > >> No, this is not iproute2/tc specfic. > > > > OK. > > > >> We named it 'tc_init_fixup' as it occurs just after the TC_INIT > >> (iptc_init/ip6tc_init) call. > >> If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' > >> or 'iptc_init_fixup'. > >> > >> This must occur after every load of entries, as the xt_bpf match needs > >> a fixup once read from kernel. > >> > >> The problem lies in the xt_bpf_info_v1 ABI. > >> See: > >> https://marc.info/?l=netfilter-devel&m=150530909630143&w=2 > > > > I see, can we get a v2 ABI that fixes this? Given this was included > > not long time ago, we can quickly deprecate this without this custom > > hook to address this. > > We can perhaps change the kernel module to ignore .fd and do a > path lookup for .path directly inside the kernel. That would not > require a v2, even. That sounds very reasonable, so we can just address this as a plain fix and pass it on to -stable. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Monday 2017-09-18 19:00, Shmulik Ladkani wrote: > >This must occur after every load of entries, as the xt_bpf match needs >a fixup once read from kernel. So you could use the check function for it, do you not? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 18, 2017 at 07:54:24PM +0200, Pablo Neira Ayuso wrote: > On Mon, Sep 18, 2017 at 01:50:32PM -0400, Willem de Bruijn wrote: > > On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote: > > >> Hi Pablo, > > >> > > >> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > >> > > >> > > > > >> > > + /* Fixes the match info after init. */ > > >> > > + void (*tc_init_fixup)(struct xt_entry_match *match); > > >> > > > >> > If this is only broken from tc ipt actions, could you fix this from > > >> > iproute2/tc instead? > > >> > > >> No, this is not iproute2/tc specfic. > > > > > > OK. > > > > > >> We named it 'tc_init_fixup' as it occurs just after the TC_INIT > > >> (iptc_init/ip6tc_init) call. > > >> If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' > > >> or 'iptc_init_fixup'. > > >> > > >> This must occur after every load of entries, as the xt_bpf match needs > > >> a fixup once read from kernel. > > >> > > >> The problem lies in the xt_bpf_info_v1 ABI. > > >> See: > > >> https://marc.info/?l=netfilter-devel&m=150530909630143&w=2 > > > > > > I see, can we get a v2 ABI that fixes this? Given this was included > > > not long time ago, we can quickly deprecate this without this custom > > > hook to address this. > > > > We can perhaps change the kernel module to ignore .fd and do a > > path lookup for .path directly inside the kernel. That would not > > require a v2, even. > > That sounds very reasonable, so we can just address this as a plain > fix and pass it on to -stable. Anyone following up with this? Thanks! -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Pablo, On Wed, 4 Oct 2017 16:33:01 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > We can perhaps change the kernel module to ignore .fd and do a > > > path lookup for .path directly inside the kernel. That would not > > > require a v2, even. > > > > That sounds very reasonable, so we can just address this as a plain > > fix and pass it on to -stable. > > Anyone following up with this? I plan to work on a fix to the v1 abi, in which the given fd is ignored. Best, Shmulik -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/xtables.h b/include/xtables.h index e9bc3b7d..687cfe9f 100644 --- a/include/xtables.h +++ b/include/xtables.h @@ -273,6 +273,9 @@ struct xtables_match { /* ip is struct ipt_ip * for example */ void (*save)(const void *ip, const struct xt_entry_match *match); + /* Fixes the match info after init. */ + void (*tc_init_fixup)(struct xt_entry_match *match); + /* Print match name or alias */ const char *(*alias)(const struct xt_entry_match *match); diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c index 49bd006f..0a6afa77 100644 --- a/iptables/ip6tables.c +++ b/iptables/ip6tables.c @@ -925,6 +925,39 @@ delete_chain6(const xt_chainlabel chain, int verbose, return ip6tc_delete_chain(chain, handle); } + +static int +tc_init_fixup_match(struct xt_entry_match *m) +{ + const struct xtables_match *match = + xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); + + if (match) { + if (match->tc_init_fixup && m->u.user.revision == match->revision) + match->tc_init_fixup(m); + } + + /* Don't stop iterating. */ + return 0; +} + +static void +tc_init_fixup(struct xtc_handle *handle) +{ + const char *chain; + + for (chain = ip6tc_first_chain(handle); + chain; + chain = ip6tc_next_chain(handle)) { + const struct ip6t_entry *entry = ip6tc_first_rule(chain, handle); + + while (entry) { + IP6T_MATCH_ITERATE(entry, tc_init_fixup_match); + entry = ip6tc_next_rule(entry, handle); + } + } +} + static int list_entries(const xt_chainlabel chain, int rulenum, int verbose, int numeric, int expanded, int linenumbers, struct xtc_handle *handle) @@ -1795,6 +1828,8 @@ int do_command6(int argc, char *argv[], char **table, "can't initialize ip6tables table `%s': %s", *table, ip6tc_strerror(errno)); + tc_init_fixup(*handle); + if (command == CMD_APPEND || command == CMD_DELETE || command == CMD_CHECK diff --git a/iptables/iptables.c b/iptables/iptables.c index 69d19fec..f220a8e4 100644 --- a/iptables/iptables.c +++ b/iptables/iptables.c @@ -909,6 +909,38 @@ delete_chain4(const xt_chainlabel chain, int verbose, return iptc_delete_chain(chain, handle); } +static int +tc_init_fixup_match(struct xt_entry_match *m) +{ + const struct xtables_match *match = + xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL); + + if (match) { + if (match->tc_init_fixup && m->u.user.revision == match->revision) + match->tc_init_fixup(m); + } + + /* Don't stop iterating. */ + return 0; +} + +static void +tc_init_fixup(struct xtc_handle *handle) +{ + const char *chain; + + for (chain = iptc_first_chain(handle); + chain; + chain = iptc_next_chain(handle)) { + const struct ipt_entry *entry = iptc_first_rule(chain, handle); + + while (entry) { + IPT_MATCH_ITERATE(entry, tc_init_fixup_match); + entry = iptc_next_rule(entry, handle); + } + } +} + static int list_entries(const xt_chainlabel chain, int rulenum, int verbose, int numeric, int expanded, int linenumbers, struct xtc_handle *handle) @@ -1781,6 +1813,8 @@ int do_command4(int argc, char *argv[], char **table, "can't initialize iptables table `%s': %s", *table, iptc_strerror(errno)); + tc_init_fixup(*handle); + if (command == CMD_APPEND || command == CMD_DELETE || command == CMD_CHECK