Re: [RFC] [PATCH 4/7] Uprobes Implementation

[Srikar Dronamraju <srikar@linux.vnet.ibm.com>]

Hi Paul, 


> > +
> > +/*
> > + * Allocate a uprobe_task object for p and add it to uproc's list.
> > + * Called with p "got" and uproc->rwsem write-locked.  Called in one of
> > + * the following cases:
> > + * - before setting the first uprobe in p's process
> > + * - we're in uprobe_report_clone() and p is the newly added thread
> > + * Returns:
> > + * - pointer to new uprobe_task on success
> > + * - NULL if t dies before we can utrace_attach it
> > + * - negative errno otherwise
> > + */
> > +static struct uprobe_task *uprobe_add_task(struct pid *p,
> > +		struct uprobe_process *uproc)
> > +{
> > +	struct uprobe_task *utask;
> > +	struct utrace_engine *engine;
> > +	struct task_struct *t = pid_task(p, PIDTYPE_PID);
> 
> What keeps the task_struct referenced by "t" from disappearing at this
> point?

We have a ref-counted pid which is used for creation of the utrace
engine. If the task_struct disappears then utrace would refuse to create
an engine and we wouldnt proceed further.  We only use the task struct
and pid only when we have a successful utrace engine.  Once utrace
engine is created,utrace guarantees us that the task will remain till
Uprobes is notified of the death/exit.

> 
> > +
> > +	if (!t)
> > +		return NULL;
> > +	utask = kzalloc(sizeof *utask, GFP_USER);
> > +	if (unlikely(utask == NULL))
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	utask->pid = p;
> > +	utask->tsk = t;
> > +	utask->state = UPTASK_RUNNING;
> > +	utask->quiescing = false;
> > +	utask->uproc = uproc;
> > +	utask->active_probe = NULL;
> > +	utask->doomed = false;
> > +	INIT_LIST_HEAD(&utask->deferred_registrations);
> > +	INIT_LIST_HEAD(&utask->delayed_signals);
> > +	INIT_LIST_HEAD(&utask->list);
> > +	list_add_tail(&utask->list, &uproc->thread_list);
> > +	uprobe_hash_utask(utask);
> > +
> > +	engine = utrace_attach_pid(p, UTRACE_ATTACH_CREATE,
> > +						p_uprobe_utrace_ops, utask);
> > +	if (IS_ERR(engine)) {
> > +		long err = PTR_ERR(engine);
> > +		printk("uprobes: utrace_attach_task failed, returned %ld\n",
> > +									err);
> > +		uprobe_free_task(utask, 0);
> > +		if (err == -ESRCH)
> > +			return NULL;
> > +		return ERR_PTR(err);
> > +	}
> > +				goto dont_add;
> > +			list_for_each_entry(utask, &uproc->thread_list, list) {
> 
> Doesn't this need to be list_for_each_entry_rcu()?
> 
> Or do you have ->thread_list protected elsewise?

thread_list is protected by write lock for uproc->rwsem.

> 
> > +				if (utask->tsk == t)
> > +					/* Already added */
> > +					goto dont_add;
> > +			}
> > +			/* Found thread/task to add. */
> > +			pid = get_pid(task_pid(t));
> > +			break;
> > +dont_add:
> > +			t = next_thread(t);
> > +		} while (t != start);
> > +	}
> > +	rcu_read_unlock();
> 
> Now that we are outside of rcu_read_lock()'s protection, the task
> indicated by "pid" might disappear, and the value of "pid" might well
> be reused.  Is this really OK?

We have a ref-counted pid; so pid should ideally not disappear.
And as I said earlier, once utrace engine gets created, we are sure that
the task struct lies till the engine gets detached. If an engine is not
created, we dont use the task struct or the pid. We piggyback on the
guarantee that utrace provides.

> 
> > +	return pid;
> > +}
> > +
> > +/*
> > + * Given a numeric thread ID, return a ref-counted struct pid for the
> > + * task-group-leader thread.
> > + */
> > +static struct pid *uprobe_get_tg_leader(pid_t p)
> > +{
> > +	struct pid *pid = NULL;
> > +
> > +	rcu_read_lock();
> > +	if (current->nsproxy)
> > +		pid = find_vpid(p);
> > +	if (pid) {
> > +		struct task_struct *t = pid_task(pid, PIDTYPE_PID);
> > +		if (t)
> > +			pid = task_tgid(t);
> > +		else
> > +			pid = NULL;
> > +	}
> > +	rcu_read_unlock();
> 
> What happens if the thread disappears at this point?  We are outside of
> rcu_read_lock() protection, so all the structures could potentially be
> freed up by other CPUs, especially if this CPU takes an interrupt or is
> preempted.
> 
> > +	return get_pid(pid);	/* null pid OK here */
> > +}


Same as above ; 

> > +/*
> > + * Signal callback:
> > + */
> > +static u32 uprobe_report_signal(u32 action,
> > +				struct utrace_engine *engine,
> > +				struct pt_regs *regs,
> > +				siginfo_t *info,
> > +				const struct k_sigaction *orig_ka,
> > +				struct k_sigaction *return_ka)
> > +{
> > +	struct uprobe_task *utask;
> > +	struct uprobe_process *uproc;
> > +	bool doomed;
> > +	enum utrace_resume_action report_action;
> > +
> > +	utask = (struct uprobe_task *)rcu_dereference(engine->data);
> 
> Are we really in an RCU read-side critical section here?

Yeah we dont need the rcu_deference here.

> 
> > +static u32 uprobe_report_quiesce(u32 action,
> > +				struct utrace_engine *engine,
> > +				unsigned long event)
> > +{
> > +	struct uprobe_task *utask;
> > +	struct uprobe_process *uproc;
> > +	bool done_quiescing = false;
> > +
> > +	utask = (struct uprobe_task *)rcu_dereference(engine->data);
> 
> Are we really in an RCU read-side critical section here?

Yeah we dont need the rcu_deference here also.

> 
> > +
> > +static u32 uprobe_exec_exit(struct utrace_engine *engine,
> > +				struct task_struct *tsk, int exit)
> > +{
> > +	struct uprobe_process *uproc;
> > +	struct uprobe_probept *ppt;
> > +	struct uprobe_task *utask;
> > +	bool utask_quiescing;
> > +
> > +	utask = (struct uprobe_task *)rcu_dereference(engine->data);
> 
> Are we really in an RCU read-side critical section here?

Yeah we dont need the rcu_deference here also.

> 
> > + *	- Provide option for child to inherit uprobes.
> > + */
> > +static u32 uprobe_report_clone(u32 action,
> > +				struct utrace_engine *engine,
> > +				unsigned long clone_flags,
> > +				struct task_struct *child)
> > +{
> > +	struct uprobe_process *uproc;
> > +	struct uprobe_task *ptask, *ctask;
> > +
> > +	ptask = (struct uprobe_task *)rcu_dereference(engine->data);
> 
> Are we really in an RCU read-side critical section here?

Yeah we dont need the rcu_deference here also.

--
Thanks and Regards
Srikar