linux-kernel - Re: [RFC PATCH 1/2] tracing/user_events: Use remote writes for event enablement

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]

Message-ID: <20221101164530.GA69@W11-BEAU-MD.localdomain>
Date:   Tue, 1 Nov 2022 09:45:30 -0700
From:   Beau Belgrave <beaub@...ux.microsoft.com>
To:     Masami Hiramatsu <mhiramat@...nel.org>
Cc:     rostedt@...dmis.org, mathieu.desnoyers@...icios.com,
        dcook@...ux.microsoft.com, alanau@...ux.microsoft.com,
        linux-trace-devel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 1/2] tracing/user_events: Use remote writes for event
 enablement

On Tue, Nov 01, 2022 at 08:55:01AM +0900, Masami Hiramatsu wrote:
> On Mon, 31 Oct 2022 09:46:03 -0700
> Beau Belgrave <beaub@...ux.microsoft.com> wrote:
> 
> > On Mon, Oct 31, 2022 at 11:47:03PM +0900, Masami Hiramatsu wrote:
> > > Hi,
> > > 
> > > I have some comments.
> > > 
> > > On Thu, 27 Oct 2022 15:40:10 -0700
> > > Beau Belgrave <beaub@...ux.microsoft.com> wrote:
> > > 
> > > [...]
> > > > @@ -1570,11 +1610,12 @@ static long user_reg_get(struct user_reg __user *ureg, struct user_reg *kreg)
> > > >   * Registers a user_event on behalf of a user process.
> > > >   */
> > > >  static long user_events_ioctl_reg(struct user_event_file_info *info,
> > > > -				  unsigned long uarg)
> > > > +				  struct file *file, unsigned long uarg)
> > > >  {
> > > >  	struct user_reg __user *ureg = (struct user_reg __user *)uarg;
> > > >  	struct user_reg reg;
> > > >  	struct user_event *user;
> > > > +	struct user_event_enabler *enabler;
> > > >  	char *name;
> > > >  	long ret;
> > > >  
> > > > @@ -1607,8 +1648,12 @@ static long user_events_ioctl_reg(struct user_event_file_info *info,
> > > >  	if (ret < 0)
> > > >  		return ret;
> > > >  
> > > > +	enabler = user_event_enabler_create(file, &reg, user);
> > > > +
> > > > +	if (!enabler)
> > > 
> > > Shouldn't we free the user_event if needed here?
> > > (I found the similar memory leak pattern in the above failure case
> > >  of the user_events_ref_add().)
> > > 
> > 
> > user_events are shared across the entire group. They cannot be cleaned
> > up until all references are gone. This is true both in this case and the
> > in the user_events_ref_add() case.
> > 
> > The pattern is to register events in the group's hashtable, then add
> > them to the local file ref array that is RCU protected. If the file ref
> > cannot be allocated, etc. the refcount on user is decremented. If we
> > cannot create an enabler, the refcount is still held until file release.
> 
> OK, when the ioctl returns, there should be 3 cases;
> 
> - Return success, a new(existing) user_event added.
> - Return error, no new user_event added.
> - Return error, a new user_event added but enabler is not initialized.
> 
> And in the last case, the new user_event will be released when user
> closes the file. Could you comment it here?
> 

Sure thing, I'll add it.

> > 
> > If the event has already been added to the local file ref array, it is
> > returned to prevent another reference.
> 
> I'm not sure this point. Did you mean returning an error to prevent
> registering the same event again?
> 

If a process uses the same fd and registers the same event multiple
times, then only 1 index is returned to the caller. If someone either
purposely or accidentally does this, the appropriate index will be
returned and we won't just keep allocating user_event objects.

However, in a threaded application, there may be situations where thread
A registers event E, and thread B registers event E as well. However,
they may pass different enable address locations. In this case you will
get 2 enablers for the single event. Right now the code does this all
the time, it does not do it only if the enable address differs.

I'm not sure how common this scenario is, but I think I should likely
check if the address is different or not before creating another enabler
in this case.

Thoughts?

> 
> > 
> > > > +		return -ENOMEM;
> > > > +
> > > >  	put_user((u32)ret, &ureg->write_index);
> > > > -	put_user(user->index, &ureg->status_bit);
> > > >  
> > > >  	return 0;
> > > >  }
> > > [...]
> > > > @@ -1849,7 +1863,6 @@ static int user_status_open(struct inode *node, struct file *file)
> > > >  
> > > >  static const struct file_operations user_status_fops = {
> > > >  	.open = user_status_open,
> > > > -	.mmap = user_status_mmap,
> > > 
> > > So, if this drops the mmap operation, can we drop the writable flag from
> > > the status tracefs file?
> > > 
> > 
> > Good catch, yes I'll remove this.
> 
> Thanks!
> 
> > 
> > > static int create_user_tracefs(void)
> > > {
> > > [...]
> > >         /* mmap with MAP_SHARED requires writable fd */
> > >         emmap = tracefs_create_file("user_events_status", TRACE_MODE_WRITE,
> > >                                     NULL, NULL, &user_status_fops);
> > > 
> > > Thank you,
> > > 
> > > -- 
> > > Masami Hiramatsu (Google) <mhiramat@...nel.org>
> > 
> > Thanks,
> > -Beau
> 
> 
> -- 
> Masami Hiramatsu (Google) <mhiramat@...nel.org>

Thanks,
-Beau