Hi,
On 30/09/17 00:10, Steffan Karger wrote:
> Hi,
>
> On 18-09-17 20:13, Antonio Quartulli wrote:
>> In the attempt of getting rid of any pf-inline.h file, we need
>> to make sure that inline functions do not trigger any circular
>> include dependency.
>>
>> For this reason, avoid pf_c2c/addr_test() to be 'struct context'
>> aware, so that pf-inline.h does not need to rely on the content
>> of openvpn.h.
>>
>> Cc: Steffan Karger <stef...@karger.me>
>> Signed-off-by: Antonio Quartulli <a...@unstable.cc>
>> ---
>>
>> v1-v3: skipped
>> v4: this is the first version of this patch, but named v4 for conveniency
>>
>>
>> src/openvpn/multi.c | 25 ++++++++++++++++++-------
>> src/openvpn/pf-inline.h | 16 ++++++++++------
>> src/openvpn/pf.c | 6 +++++-
>> 3 files changed, 33 insertions(+), 14 deletions(-)
>>
>> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
>> index c798c438..641b825a 100644
>> --- a/src/openvpn/multi.c
>> +++ b/src/openvpn/multi.c
>> @@ -2233,7 +2233,10 @@ multi_bcast(struct multi_context *m,
>> #ifdef ENABLE_PF
>> if (sender_instance)
>> {
>> - if (!pf_c2c_test(&sender_instance->context,
> &mi->context, "bcast_c2c"))
>> + if (!pf_c2c_test(&sender_instance->context.c2.pf,
>> + &sender_instance->context,
>> + &mi->context.c2.pf, &mi->context,
>> + "bcast_c2c"))
>> {
>> msg(D_PF_DROPPED_BCAST, "PF: client[%s] ->
> client[%s] packet dropped by BCAST packet filter",
>> mi_prefix(sender_instance),
>> @@ -2243,7 +2246,8 @@ multi_bcast(struct multi_context *m,
>> }
>> if (sender_addr)
>> {
>> - if (!pf_addr_test(&mi->context, sender_addr,
> "bcast_src_addr"))
>> + if (!pf_addr_test(&mi->context.c2.pf, &mi->context,
>> + sender_addr, "bcast_src_addr"))
>> {
>> struct gc_arena gc = gc_new();
>> msg(D_PF_DROPPED_BCAST, "PF: addr[%s] ->
> client[%s] packet dropped by BCAST packet filter",
>> @@ -2602,7 +2606,8 @@ multi_process_incoming_link(struct multi_context
> *m, struct multi_instance *inst
>> if (mi)
>> {
>> #ifdef ENABLE_PF
>> - if (!pf_c2c_test(c, &mi->context, "tun_c2c"))
>> + if (!pf_c2c_test(&c->c2.pf, c,
> &mi->context.c2.pf,
>> + &mi->context, "tun_c2c"))
>> {
>> msg(D_PF_DROPPED, "PF: client ->
> client[%s] packet dropped by TUN packet filter",
>> mi_prefix(mi));
>> @@ -2618,7 +2623,8 @@ multi_process_incoming_link(struct multi_context
> *m, struct multi_instance *inst
>> }
>> }
>> #ifdef ENABLE_PF
>> - if (c->c2.to_tun.len && !pf_addr_test(c, &dest,
> "tun_dest_addr"))
>> + if (c->c2.to_tun.len && !pf_addr_test(&c->c2.pf, c,
> &dest,
>> + "tun_dest_addr"))
>> {
>> msg(D_PF_DROPPED, "PF: client -> addr[%s] packet
> dropped by TUN packet filter",
>> mroute_addr_print_ex(&dest, MAPF_SHOW_ARP, &gc));
>> @@ -2663,7 +2669,10 @@ multi_process_incoming_link(struct
> multi_context *m, struct multi_instance *inst
>> if (mi)
>> {
>> #ifdef ENABLE_PF
>> - if (!pf_c2c_test(c, &mi->context,
> "tap_c2c"))
>> + if (!pf_c2c_test(&c->c2.pf, c,
>> + &mi->context.c2.pf,
>> + &mi->context,
>> + "tap_c2c"))
>> {
>> msg(D_PF_DROPPED, "PF: client
> -> client[%s] packet dropped by TAP packet filter",
>> mi_prefix(mi));
>> @@ -2679,7 +2688,9 @@ multi_process_incoming_link(struct multi_context
> *m, struct multi_instance *inst
>> }
>> }
>> #ifdef ENABLE_PF
>> - if (c->c2.to_tun.len && !pf_addr_test(c,
> &edest, "tap_dest_addr"))
>> + if (c->c2.to_tun.len &&
> !pf_addr_test(&c->c2.pf, c,
>> + &edest,
>> +
> "tap_dest_addr"))
>> {
>> msg(D_PF_DROPPED, "PF: client -> addr[%s]
> packet dropped by TAP packet filter",
>> mroute_addr_print_ex(&edest,
> MAPF_SHOW_ARP, &gc));
>> @@ -2792,7 +2803,7 @@ multi_process_incoming_tun(struct multi_context
> *m, const unsigned int mpp_flags
>> set_prefix(m->pending);
>>
>> #ifdef ENABLE_PF
>> - if (!pf_addr_test(c, e2, "tun_tap_src_addr"))
>> + if (!pf_addr_test(&c->c2.pf, c, e2,
> "tun_tap_src_addr"))
>> {
>> msg(D_PF_DROPPED, "PF: addr[%s] -> client
> packet dropped by packet filter",
>> mroute_addr_print_ex(&src, MAPF_SHOW_ARP,
> &gc));
>> diff --git a/src/openvpn/pf-inline.h b/src/openvpn/pf-inline.h
>> index ac19ac4c..7e38e674 100644
>> --- a/src/openvpn/pf-inline.h
>> +++ b/src/openvpn/pf-inline.h
>> @@ -31,20 +31,24 @@
>> #define PCT_SRC 1
>> #define PCT_DEST 2
>> static inline bool
>> -pf_c2c_test(const struct context *src, const struct context *dest,
> const char *prefix)
>> +pf_c2c_test(const struct pf_context *src_pf, const struct context *src,
>> + const struct pf_context *dest_pf, const struct context *dest,
>> + const char *prefix)
>
> If you make this function take a const struct tls_multi * as 2nd and 4th
> argument, you don't have to change the pf_cn_test function prototype
> (and it doesn't have to know about struct context internals, which is
> useful for testing modules separately).
Sorry for forgetting about this comment. This is actually a good idea!
v5 will come soon.
Cheers,
>
>> {
>> - bool pf_cn_test(struct pf_set *pfs, const struct tls_multi *tm,
> const int type, const char *prefix);
>> + bool pf_cn_test(const struct context *src, const struct context
> *dest,
>> + const int type, const char *prefix);
>>
>> - return (!src->c2.pf.enabled || pf_cn_test(src->c2.pf.pfs,
> dest->c2.tls_multi, PCT_DEST, prefix))
>> - && (!dest->c2.pf.enabled || pf_cn_test(dest->c2.pf.pfs,
> src->c2.tls_multi, PCT_SRC, prefix));
>> + return (!src_pf->enabled || pf_cn_test(src, dest, PCT_DEST, prefix))
>> + && (!dest_pf->enabled || pf_cn_test(dest, src, PCT_SRC,
> prefix));
>> }
>>
>> static inline bool
>> -pf_addr_test(const struct context *src, const struct mroute_addr
> *dest, const char *prefix)
>> +pf_addr_test(const struct pf_context *src_pf, const struct context *src,
>> + const struct mroute_addr *dest, const char *prefix)
>> {
>> bool pf_addr_test_dowork(const struct context *src, const struct
> mroute_addr *dest, const char *prefix);
>>
>> - if (src->c2.pf.enabled)
>> + if (src_pf->enabled)
>> {
>> return pf_addr_test_dowork(src, dest, prefix);
>> }
>> diff --git a/src/openvpn/pf.c b/src/openvpn/pf.c
>> index 5cb002bf..135cb15d 100644
>> --- a/src/openvpn/pf.c
>> +++ b/src/openvpn/pf.c
>> @@ -442,8 +442,12 @@ lookup_cn_rule(struct hash *h, const char *cn,
> const uint32_t cn_hash)
>> }
>>
>> bool
>> -pf_cn_test(struct pf_set *pfs, const struct tls_multi *tm, const int
> type, const char *prefix)
>> +pf_cn_test(const struct context *src, const struct context *dest,
>> + const int type, const char *prefix)
>> {
>> + struct pf_set *pfs = src->c2.pf.pfs;
>> + struct tls_multi *tm = dest->c2.tls_multi;
>> +
>> if (pfs && !pfs->kill)
>> {
>> const char *cn;
>>
>
> Otherwise this looks good to me.
>
> -Steffan
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
--
Antonio Quartulli
signature.asc
Description: OpenPGP digital signature
------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
