Re: [PATCH -tip v2 3/3] kprobes: Set IPMODIFY flag only if the probe can change regs->ip

[Namhyung Kim <namhyung@kernel.org>]

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;
> +}