Jan Engelhardt wrote:
> diff --git a/net/ipv4/netfilter/ipt_LOG.c b/net/ipv4/netfilter/ipt_LOG.c
> index c9ee5c4..33213d6 100644
> --- a/net/ipv4/netfilter/ipt_LOG.c
> +++ b/net/ipv4/netfilter/ipt_LOG.c
> @@ -445,7 +445,7 @@ static int log_tg_check(const struct xt_tgchk_param *par)
>
> if (loginfo->level >= 8) {
> pr_debug("level %u >= 8\n", loginfo->level);
> - return false;
> + return -EDOM;
>
Mhh is that really a "Math argument out of domain of func"? ERANGE seems
slightly
better fitting to me.
> }
> if (loginfo->prefix[sizeof(loginfo->prefix)-1] != '\0') {
> pr_debug("prefix is not null-terminated\n");
> diff --git a/net/netfilter/x_tables.c b/net/netfilter/x_tables.c
> index 4e84d8a..6ead6e0 100644
> --- a/net/netfilter/x_tables.c
> +++ b/net/netfilter/x_tables.c
> @@ -363,6 +363,8 @@ static char *textify_hooks(char *buf, size_t size, unsigned int mask)
> int xt_check_match(struct xt_mtchk_param *par,
> unsigned int size, u_int8_t proto, bool inv_proto)
> {
> + int ret;
> +
> if (XT_ALIGN(par->match->matchsize) != size &&
> par->match->matchsize != -1) {
> /*
> @@ -399,8 +401,13 @@ int xt_check_match(struct xt_mtchk_param *par,
> par->match->proto);
> return -EINVAL;
> }
> - if (par->match->checkentry != NULL && !par->match->checkentry(par))
> - return -EINVAL;
> + if (par->match->checkentry != NULL) {
> + ret = par->match->checkentry(par);
> + if (ret < 0)
> + return ret;
> + else if (ret == 0)
> + return -EINVAL;
>
I'd suggest to do the true/false conversion before this patch. Currently I
don't understand the split of these patches, this one keeps lots of boolean
return values and initializations, but changes some.
> --- a/net/netfilter/xt_CONNSECMARK.c
> +++ b/net/netfilter/xt_CONNSECMARK.c
> @@ -87,6 +87,7 @@ connsecmark_tg(struct sk_buff *skb, const struct xt_target_param *par)
> static int connsecmark_tg_check(const struct xt_tgchk_param *par)
> {
> const struct xt_connsecmark_target_info *info = par->targinfo;
> + int ret;
>
> if (strcmp(par->table, "mangle") != 0 &&
> strcmp(par->table, "security") != 0) {
> @@ -102,13 +103,14 @@ static int connsecmark_tg_check(const struct xt_tgchk_param *par)
>
> default:
> pr_info("invalid mode: %hu\n", info->mode);
> - return false;
> + return -EDOM;
>
Actually EINVAL does seem like the best fit here.
>
> diff --git a/net/netfilter/xt_LED.c b/net/netfilter/xt_LED.c
> index 1a3e3dd..899da12 100644
> --- a/net/netfilter/xt_LED.c
> +++ b/net/netfilter/xt_LED.c
> @@ -93,7 +93,7 @@ static int led_tg_check(const struct xt_tgchk_param *par)
>
> ledinternal = kzalloc(sizeof(struct xt_led_info_internal), GFP_KERNEL);
> if (!ledinternal)
> - return false;
> + return -ENOMEM;
>
> ledinternal->netfilter_led_trigger.name = ledinfo->id;
>
> @@ -102,7 +102,8 @@ static int led_tg_check(const struct xt_tgchk_param *par)
> pr_warning("led_trigger_register() failed\n");
> if (err == -EEXIST)
> pr_warning("Trigger name is already in use.\n");
> - goto exit_alloc;
> + kfree(ledinternal);
> + return err;
> }
>
> /* See if we need to set up a timer */
> @@ -111,13 +112,7 @@ static int led_tg_check(const struct xt_tgchk_param *par)
> (unsigned long)ledinfo);
>
> ledinfo->internal_data = ledinternal;
> -
> return true;
>
^^ return true? I'd also prefer to keep the goto-based error handling.
> -
> -exit_alloc:
> - kfree(ledinternal);
> -
> - return false;
> }
>
> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index 48f8e4f..6525eee 100644
> --- a/net/netfilter/xt_SECMARK.c
> +++ b/net/netfilter/xt_SECMARK.c
> @@ -50,7 +50,7 @@ secmark_tg(struct sk_buff *skb, const struct xt_target_param *par)
> return XT_CONTINUE;
> }
>
> -static bool checkentry_selinux(struct xt_secmark_target_info *info)
> +static int checkentry_selinux(struct xt_secmark_target_info *info)
> {
> int err;
> struct xt_secmark_target_selinux_info *sel = &info->u.sel;
> @@ -62,18 +62,18 @@ static bool checkentry_selinux(struct xt_secmark_target_info *info)
> if (err == -EINVAL)
> pr_info("invalid SELinux context \'%s\'\n",
> sel->selctx);
> - return false;
> + return err;
> }
>
> if (!sel->selsid) {
> pr_info("unable to map SELinux context \'%s\'\n", sel->selctx);
> - return false;
> + return -ENOENT;
> }
>
> err = selinux_secmark_relabel_packet_permission(sel->selsid);
> if (err) {
> pr_info("unable to obtain relabeling permission\n");
> - return false;
> + return err;
> }
>
> selinux_secmark_refcount_inc();
>
Missing conversion of "return true" at the end?
> @@ -83,6 +83,7 @@ static bool checkentry_selinux(struct xt_secmark_target_info *info)
> static int secmark_tg_check(const struct xt_tgchk_param *par)
> {
> struct xt_secmark_target_info *info = par->targinfo;
> + int err;
>
> if (strcmp(par->table, "mangle") != 0 &&
> strcmp(par->table, "security") != 0) {
> @@ -99,13 +100,14 @@ static int secmark_tg_check(const struct xt_tgchk_param *par)
>
> switch (info->mode) {
> case SECMARK_MODE_SEL:
> - if (!checkentry_selinux(info))
> - return false;
> + err = checkentry_selinux(info);
> + if (err <= 0)
>
<= ?
> + return err;
> break;
>
> default:
> pr_info("invalid mode: %hu\n", info->mode);
> - return false;
> + return -EDOM;
> }
>
> if (!mode)
>
I stopped here since I don't understand the logic this partial conversion
follows.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
