lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 19 Jun 2014 21:34:31 +0900
From:	Namhyung Kim <namhyung@...nel.org>
To:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
Cc:	Steven Rostedt <rostedt@...dmis.org>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Ingo Molnar <mingo@...nel.org>,
	Ananth N Mavinakayanahalli <ananth@...ibm.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH -tip v2 3/3] kprobes: Set IPMODIFY flag only if the
 probe can change regs->ip

Hi Masami,

2014-06-17 (화), 11:04 +0000, Masami Hiramatsu:
> +static int __ftrace_add_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> +				  int *ref)
> +{
> +	int ret;
> +
> +	/* Try to set given ip to filter */
> +	ret = ftrace_set_filter_ip(ops, ip, 0, 0);
> +	if (ret >= 0) {

Hmm.. this doesn't look comfortable.  What not using usual pattern?

	if (ret < 0)
		return ret;

This way we can reduce a indent level.


> +		(*ref)++;
> +		if (*ref == 1) {
> +			ret = register_ftrace_function(ops);
> +			if (ret < 0) {
> +				/* Rollback refcounter and filter */
> +				(*ref)--;
> +				ftrace_set_filter_ip(ops, ip, 1, 0);
> +			}
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static int __ftrace_remove_filter_ip(struct ftrace_ops *ops, unsigned long ip,
> +				     int *ref)
> +{
> +	int ret = 0;
> +
> +	(*ref)--;
> +	if (*ref == 0)
> +		ret = unregister_ftrace_function(ops);
> +	if (ret >= 0)
> +		/* Try to remove given ip to filter */
> +		ret = ftrace_set_filter_ip(ops, ip, 1, 0);

I think any failure at this time can be problematic.  Since we already
unregistered the ops but the refcount will get increased again, future
attemp to register won't enable the ops anymore IMHO.

I think it'd better just ignoring faiure of filter removal here.  We'll
miss a filter entry but it'll be usable anyway.

What about this?

static int __ftrace_remove_filter_ip(...)
{
	if (*ref == 1) {
		int ret = unregister_ftrace_function(ops);
		if (ret < 0)
			return ret;

		ftrace_set_filter_ip(ops, ip, 1, 0);
	}

	(*ref)--;
	return 0;
}

Thanks,
Namhyung


> +	if (ret < 0)	/* Rollback refcounter if an error occurs */
> +		(*ref)++;
> +
> +	return ret;
> +}


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists