From: Tom Rix <[email protected]>
Extends
commit 83ace77f5117 ("netfilter: ctnetlink: remove get_ct indirection")
Add some compile time checks by following the ct and ctinfo variables
that are only set when CONFIG_NF_CONNTRACK is enabled.
In nfulnl_log_packet(), ct is only set when CONFIG_NF_CONNTRACK
is enabled. ct's later use in __build_packet_message() is only
meaningful when CONFIG_NF_CONNTRACK is enabled, so add a check.
In nfqnl_build_packet_message(), ct and ctinfo are only set when
CONFIG_NF_CONNTRACK is enabled. Add a check for their decl and use.
nfqnl_ct_parse() is a static function, move the check to the whole
function.
In nfqa_parse_bridge(), ct and ctinfo are only set by the only
call to nfqnl_ct_parse(), so add a check for their decl and use.
Consistently initialize ctinfo to 0.
Signed-off-by: Tom Rix <[email protected]>
---
net/netfilter/nfnetlink_log.c | 4 +++-
net/netfilter/nfnetlink_queue.c | 18 +++++++++++++-----
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index ae9c0756bba59..e79d152184b71 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -627,9 +627,11 @@ __build_packet_message(struct nfnl_log_net *log,
htonl(atomic_inc_return(&log->global_seq))))
goto nla_put_failure;
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
if (ct && nfnl_ct->build(inst->skb, ct, ctinfo,
NFULA_CT, NFULA_CT_INFO) < 0)
goto nla_put_failure;
+#endif
if ((pf == NFPROTO_NETDEV || pf == NFPROTO_BRIDGE) &&
nfulnl_put_bridge(inst, skb) < 0)
@@ -689,7 +691,7 @@ nfulnl_log_packet(struct net *net,
struct nfnl_log_net *log = nfnl_log_pernet(net);
const struct nfnl_ct_hook *nfnl_ct = NULL;
struct nf_conn *ct = NULL;
- enum ip_conntrack_info ctinfo;
+ enum ip_conntrack_info ctinfo = 0;
if (li_user && li_user->type == NF_LOG_TYPE_ULOG)
li = li_user;
diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
index 44c3de176d186..d59cae7561bf8 100644
--- a/net/netfilter/nfnetlink_queue.c
+++ b/net/netfilter/nfnetlink_queue.c
@@ -386,8 +386,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
struct sk_buff *entskb = entry->skb;
struct net_device *indev;
struct net_device *outdev;
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
struct nf_conn *ct = NULL;
enum ip_conntrack_info ctinfo = 0;
+#endif
struct nfnl_ct_hook *nfnl_ct;
bool csum_verify;
char *secdata = NULL;
@@ -595,8 +597,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
goto nla_put_failure;
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
goto nla_put_failure;
+#endif
if (cap_len > data_len &&
nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
@@ -1104,13 +1108,13 @@ static int nfqnl_recv_verdict_batch(struct sk_buff *skb,
return 0;
}
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
const struct nlmsghdr *nlh,
const struct nlattr * const nfqa[],
struct nf_queue_entry *entry,
enum ip_conntrack_info *ctinfo)
{
-#if IS_ENABLED(CONFIG_NF_CONNTRACK)
struct nf_conn *ct;
ct = nf_ct_get(entry->skb, ctinfo);
@@ -1125,10 +1129,8 @@ static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
NETLINK_CB(entry->skb).portid,
nlmsg_report(nlh));
return ct;
-#else
- return NULL;
-#endif
}
+#endif
static int nfqa_parse_bridge(struct nf_queue_entry *entry,
const struct nlattr * const nfqa[])
@@ -1172,11 +1174,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
struct nfnl_queue_net *q = nfnl_queue_pernet(info->net);
u_int16_t queue_num = ntohs(info->nfmsg->res_id);
struct nfqnl_msg_verdict_hdr *vhdr;
- enum ip_conntrack_info ctinfo;
struct nfqnl_instance *queue;
struct nf_queue_entry *entry;
struct nfnl_ct_hook *nfnl_ct;
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
struct nf_conn *ct = NULL;
+ enum ip_conntrack_info ctinfo = 0;
+#endif
unsigned int verdict;
int err;
@@ -1198,11 +1202,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
/* rcu lock already held from nfnl->call_rcu. */
nfnl_ct = rcu_dereference(nfnl_ct_hook);
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
if (nfqa[NFQA_CT]) {
if (nfnl_ct != NULL)
ct = nfqnl_ct_parse(nfnl_ct, info->nlh, nfqa, entry,
&ctinfo);
}
+#endif
if (entry->state.pf == PF_BRIDGE) {
err = nfqa_parse_bridge(entry, nfqa);
@@ -1218,8 +1224,10 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
payload_len, entry, diff) < 0)
verdict = NF_DROP;
+#if IS_ENABLED(CONFIG_NF_CONNTRACK)
if (ct && diff)
nfnl_ct->seq_adjust(entry->skb, ct, ctinfo, diff);
+#endif
}
if (nfqa[NFQA_MARK])
--
2.26.3
Hi,
On Sat, Dec 25, 2021 at 09:37:44AM -0800, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> Extends
> commit 83ace77f5117 ("netfilter: ctnetlink: remove get_ct indirection")
>
> Add some compile time checks by following the ct and ctinfo variables
> that are only set when CONFIG_NF_CONNTRACK is enabled.
>
> In nfulnl_log_packet(), ct is only set when CONFIG_NF_CONNTRACK
> is enabled. ct's later use in __build_packet_message() is only
> meaningful when CONFIG_NF_CONNTRACK is enabled, so add a check.
>
> In nfqnl_build_packet_message(), ct and ctinfo are only set when
> CONFIG_NF_CONNTRACK is enabled. Add a check for their decl and use.
>
> nfqnl_ct_parse() is a static function, move the check to the whole
> function.
>
> In nfqa_parse_bridge(), ct and ctinfo are only set by the only
> call to nfqnl_ct_parse(), so add a check for their decl and use.
>
> Consistently initialize ctinfo to 0.
Are compile warning being trigger without this patch, maybe with
CONFIG_NF_CONNTRACK=n?
> Signed-off-by: Tom Rix <[email protected]>
> ---
> net/netfilter/nfnetlink_log.c | 4 +++-
> net/netfilter/nfnetlink_queue.c | 18 +++++++++++++-----
> 2 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index ae9c0756bba59..e79d152184b71 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -627,9 +627,11 @@ __build_packet_message(struct nfnl_log_net *log,
> htonl(atomic_inc_return(&log->global_seq))))
> goto nla_put_failure;
>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> if (ct && nfnl_ct->build(inst->skb, ct, ctinfo,
> NFULA_CT, NFULA_CT_INFO) < 0)
> goto nla_put_failure;
> +#endif
>
> if ((pf == NFPROTO_NETDEV || pf == NFPROTO_BRIDGE) &&
> nfulnl_put_bridge(inst, skb) < 0)
> @@ -689,7 +691,7 @@ nfulnl_log_packet(struct net *net,
> struct nfnl_log_net *log = nfnl_log_pernet(net);
> const struct nfnl_ct_hook *nfnl_ct = NULL;
> struct nf_conn *ct = NULL;
> - enum ip_conntrack_info ctinfo;
> + enum ip_conntrack_info ctinfo = 0;
>
> if (li_user && li_user->type == NF_LOG_TYPE_ULOG)
> li = li_user;
> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
> index 44c3de176d186..d59cae7561bf8 100644
> --- a/net/netfilter/nfnetlink_queue.c
> +++ b/net/netfilter/nfnetlink_queue.c
> @@ -386,8 +386,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> struct sk_buff *entskb = entry->skb;
> struct net_device *indev;
> struct net_device *outdev;
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> struct nf_conn *ct = NULL;
> enum ip_conntrack_info ctinfo = 0;
> +#endif
> struct nfnl_ct_hook *nfnl_ct;
> bool csum_verify;
> char *secdata = NULL;
> @@ -595,8 +597,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
> if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
> goto nla_put_failure;
>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
> goto nla_put_failure;
> +#endif
>
> if (cap_len > data_len &&
> nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
> @@ -1104,13 +1108,13 @@ static int nfqnl_recv_verdict_batch(struct sk_buff *skb,
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
> const struct nlmsghdr *nlh,
> const struct nlattr * const nfqa[],
> struct nf_queue_entry *entry,
> enum ip_conntrack_info *ctinfo)
> {
> -#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> struct nf_conn *ct;
>
> ct = nf_ct_get(entry->skb, ctinfo);
> @@ -1125,10 +1129,8 @@ static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
> NETLINK_CB(entry->skb).portid,
> nlmsg_report(nlh));
> return ct;
> -#else
> - return NULL;
> -#endif
> }
> +#endif
>
> static int nfqa_parse_bridge(struct nf_queue_entry *entry,
> const struct nlattr * const nfqa[])
> @@ -1172,11 +1174,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
> struct nfnl_queue_net *q = nfnl_queue_pernet(info->net);
> u_int16_t queue_num = ntohs(info->nfmsg->res_id);
> struct nfqnl_msg_verdict_hdr *vhdr;
> - enum ip_conntrack_info ctinfo;
> struct nfqnl_instance *queue;
> struct nf_queue_entry *entry;
> struct nfnl_ct_hook *nfnl_ct;
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> struct nf_conn *ct = NULL;
> + enum ip_conntrack_info ctinfo = 0;
> +#endif
> unsigned int verdict;
> int err;
>
> @@ -1198,11 +1202,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
> /* rcu lock already held from nfnl->call_rcu. */
> nfnl_ct = rcu_dereference(nfnl_ct_hook);
>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> if (nfqa[NFQA_CT]) {
> if (nfnl_ct != NULL)
> ct = nfqnl_ct_parse(nfnl_ct, info->nlh, nfqa, entry,
> &ctinfo);
> }
> +#endif
>
> if (entry->state.pf == PF_BRIDGE) {
> err = nfqa_parse_bridge(entry, nfqa);
> @@ -1218,8 +1224,10 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
> payload_len, entry, diff) < 0)
> verdict = NF_DROP;
>
> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
> if (ct && diff)
> nfnl_ct->seq_adjust(entry->skb, ct, ctinfo, diff);
> +#endif
> }
>
> if (nfqa[NFQA_MARK])
> --
> 2.26.3
>
On 1/11/22 1:46 AM, Pablo Neira Ayuso wrote:
> Hi,
>
> On Sat, Dec 25, 2021 at 09:37:44AM -0800, [email protected] wrote:
>> From: Tom Rix <[email protected]>
>>
>> Extends
>> commit 83ace77f5117 ("netfilter: ctnetlink: remove get_ct indirection")
>>
>> Add some compile time checks by following the ct and ctinfo variables
>> that are only set when CONFIG_NF_CONNTRACK is enabled.
>>
>> In nfulnl_log_packet(), ct is only set when CONFIG_NF_CONNTRACK
>> is enabled. ct's later use in __build_packet_message() is only
>> meaningful when CONFIG_NF_CONNTRACK is enabled, so add a check.
>>
>> In nfqnl_build_packet_message(), ct and ctinfo are only set when
>> CONFIG_NF_CONNTRACK is enabled. Add a check for their decl and use.
>>
>> nfqnl_ct_parse() is a static function, move the check to the whole
>> function.
>>
>> In nfqa_parse_bridge(), ct and ctinfo are only set by the only
>> call to nfqnl_ct_parse(), so add a check for their decl and use.
>>
>> Consistently initialize ctinfo to 0.
> Are compile warning being trigger without this patch, maybe with
> CONFIG_NF_CONNTRACK=n?
No compiler warnings, this was found by visual inspection.
Robot says to entend more, so I want to make sure a human is also
interested.
Tom
>
>> Signed-off-by: Tom Rix <[email protected]>
>> ---
>> net/netfilter/nfnetlink_log.c | 4 +++-
>> net/netfilter/nfnetlink_queue.c | 18 +++++++++++++-----
>> 2 files changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>> index ae9c0756bba59..e79d152184b71 100644
>> --- a/net/netfilter/nfnetlink_log.c
>> +++ b/net/netfilter/nfnetlink_log.c
>> @@ -627,9 +627,11 @@ __build_packet_message(struct nfnl_log_net *log,
>> htonl(atomic_inc_return(&log->global_seq))))
>> goto nla_put_failure;
>>
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> if (ct && nfnl_ct->build(inst->skb, ct, ctinfo,
>> NFULA_CT, NFULA_CT_INFO) < 0)
>> goto nla_put_failure;
>> +#endif
>>
>> if ((pf == NFPROTO_NETDEV || pf == NFPROTO_BRIDGE) &&
>> nfulnl_put_bridge(inst, skb) < 0)
>> @@ -689,7 +691,7 @@ nfulnl_log_packet(struct net *net,
>> struct nfnl_log_net *log = nfnl_log_pernet(net);
>> const struct nfnl_ct_hook *nfnl_ct = NULL;
>> struct nf_conn *ct = NULL;
>> - enum ip_conntrack_info ctinfo;
>> + enum ip_conntrack_info ctinfo = 0;
>>
>> if (li_user && li_user->type == NF_LOG_TYPE_ULOG)
>> li = li_user;
>> diff --git a/net/netfilter/nfnetlink_queue.c b/net/netfilter/nfnetlink_queue.c
>> index 44c3de176d186..d59cae7561bf8 100644
>> --- a/net/netfilter/nfnetlink_queue.c
>> +++ b/net/netfilter/nfnetlink_queue.c
>> @@ -386,8 +386,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> struct sk_buff *entskb = entry->skb;
>> struct net_device *indev;
>> struct net_device *outdev;
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> struct nf_conn *ct = NULL;
>> enum ip_conntrack_info ctinfo = 0;
>> +#endif
>> struct nfnl_ct_hook *nfnl_ct;
>> bool csum_verify;
>> char *secdata = NULL;
>> @@ -595,8 +597,10 @@ nfqnl_build_packet_message(struct net *net, struct nfqnl_instance *queue,
>> if (seclen && nla_put(skb, NFQA_SECCTX, seclen, secdata))
>> goto nla_put_failure;
>>
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> if (ct && nfnl_ct->build(skb, ct, ctinfo, NFQA_CT, NFQA_CT_INFO) < 0)
>> goto nla_put_failure;
>> +#endif
>>
>> if (cap_len > data_len &&
>> nla_put_be32(skb, NFQA_CAP_LEN, htonl(cap_len)))
>> @@ -1104,13 +1108,13 @@ static int nfqnl_recv_verdict_batch(struct sk_buff *skb,
>> return 0;
>> }
>>
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
>> const struct nlmsghdr *nlh,
>> const struct nlattr * const nfqa[],
>> struct nf_queue_entry *entry,
>> enum ip_conntrack_info *ctinfo)
>> {
>> -#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> struct nf_conn *ct;
>>
>> ct = nf_ct_get(entry->skb, ctinfo);
>> @@ -1125,10 +1129,8 @@ static struct nf_conn *nfqnl_ct_parse(struct nfnl_ct_hook *nfnl_ct,
>> NETLINK_CB(entry->skb).portid,
>> nlmsg_report(nlh));
>> return ct;
>> -#else
>> - return NULL;
>> -#endif
>> }
>> +#endif
>>
>> static int nfqa_parse_bridge(struct nf_queue_entry *entry,
>> const struct nlattr * const nfqa[])
>> @@ -1172,11 +1174,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
>> struct nfnl_queue_net *q = nfnl_queue_pernet(info->net);
>> u_int16_t queue_num = ntohs(info->nfmsg->res_id);
>> struct nfqnl_msg_verdict_hdr *vhdr;
>> - enum ip_conntrack_info ctinfo;
>> struct nfqnl_instance *queue;
>> struct nf_queue_entry *entry;
>> struct nfnl_ct_hook *nfnl_ct;
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> struct nf_conn *ct = NULL;
>> + enum ip_conntrack_info ctinfo = 0;
>> +#endif
>> unsigned int verdict;
>> int err;
>>
>> @@ -1198,11 +1202,13 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
>> /* rcu lock already held from nfnl->call_rcu. */
>> nfnl_ct = rcu_dereference(nfnl_ct_hook);
>>
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> if (nfqa[NFQA_CT]) {
>> if (nfnl_ct != NULL)
>> ct = nfqnl_ct_parse(nfnl_ct, info->nlh, nfqa, entry,
>> &ctinfo);
>> }
>> +#endif
>>
>> if (entry->state.pf == PF_BRIDGE) {
>> err = nfqa_parse_bridge(entry, nfqa);
>> @@ -1218,8 +1224,10 @@ static int nfqnl_recv_verdict(struct sk_buff *skb, const struct nfnl_info *info,
>> payload_len, entry, diff) < 0)
>> verdict = NF_DROP;
>>
>> +#if IS_ENABLED(CONFIG_NF_CONNTRACK)
>> if (ct && diff)
>> nfnl_ct->seq_adjust(entry->skb, ct, ctinfo, diff);
>> +#endif
>> }
>>
>> if (nfqa[NFQA_MARK])
>> --
>> 2.26.3
>>
Tom Rix <[email protected]> wrote:
>
> On 1/11/22 1:46 AM, Pablo Neira Ayuso wrote:
> > Hi,
> >
> > On Sat, Dec 25, 2021 at 09:37:44AM -0800, [email protected] wrote:
> > > From: Tom Rix <[email protected]>
> > >
> > > Extends
> > > commit 83ace77f5117 ("netfilter: ctnetlink: remove get_ct indirection")
> > >
> > > Add some compile time checks by following the ct and ctinfo variables
> > > that are only set when CONFIG_NF_CONNTRACK is enabled.
> > >
> > > In nfulnl_log_packet(), ct is only set when CONFIG_NF_CONNTRACK
> > > is enabled. ct's later use in __build_packet_message() is only
> > > meaningful when CONFIG_NF_CONNTRACK is enabled, so add a check.
> > >
> > > In nfqnl_build_packet_message(), ct and ctinfo are only set when
> > > CONFIG_NF_CONNTRACK is enabled. Add a check for their decl and use.
> > >
> > > nfqnl_ct_parse() is a static function, move the check to the whole
> > > function.
> > >
> > > In nfqa_parse_bridge(), ct and ctinfo are only set by the only
> > > call to nfqnl_ct_parse(), so add a check for their decl and use.
> > >
> > > Consistently initialize ctinfo to 0.
> > Are compile warning being trigger without this patch, maybe with
> > CONFIG_NF_CONNTRACK=n?
>
> No compiler warnings, this was found by visual inspection.
>
> Robot says to entend more, so I want to make sure a human is also
> interested.
I hoped compiler was able to remove that without aid of preprocessor :/