Re: [PATCH] per-user signal pending and message queue limits

[Marcelo Tosatti <marcelo.tosatti@cyclades.com>]

> This BUG() is too easy to trigger, e.g. user creates mqueue, logs out,
> root comes by later and cleans up...BUG().  Simply caching user directly
> eliminates this altogether.

And with user_struct->__count you deal with that, yes?

Caching the user_struct directly is indeed much nicer.

> 
> Some inconsistent use of p and current below.
> 
> >  	struct msg_msg **msgs = NULL;
> >  	struct mq_attr attr;
> >  	int ret;
> > @@ -553,15 +578,26 @@
> >  					attr.mq_msgsize > msgsize_max)
> >  				return ERR_PTR(-EINVAL);
> >  		}
> > +	  	if(p->user->msg_queues+ ((attr.mq_maxmsg * sizeof(struct msg_msg *)
> > +				+ (attr.mq_maxmsg * attr.mq_msgsize)))
> > +			  >= p->rlim[RLIMIT_MSGQUEUE].rlim_cur)
> 
> Hrm, this thing can overflow.  Seems like the hard maxes should be
> smaller.  As it stands, looks like the hard max mq_msgsize that root
> could setup is INT_MAX.

Eek. Just decreasing max mq_msgsize to something _much_ smaller is ok, isnt it? 

Like, say, 64MB.

> > +			return ERR_PTR(-ENOMEM);
> > +
> >  		msgs = kmalloc(attr.mq_maxmsg * sizeof(*msgs), GFP_KERNEL);
> >  		if (!msgs)
> >  			return ERR_PTR(-ENOMEM);
> > +
> > +		spin_lock(&mq_lock);
> > +		current->user->msg_queues += (attr.mq_maxmsg * sizeof(*msgs) +
> > +					(attr.mq_maxmsg * attr.mq_msgsize));
> > +		spin_unlock(&mq_lock);
> 
> This path means the user is penalized for the mq_attr sized accounting,
> plus the default sized accounting which happens later in mqueue_get_inode().
> It is removed below, but as mentioned above, this could incorrectly
> cause mq_open() to fail.

OK!

> >  	} else {
> >  		msgs = NULL;
> >  	}
> >  
> >  	ret = vfs_create(dir->d_inode, dentry, mode, NULL);
> >  	if (ret) {
> > +		/* kfree(msgs): msgs can be NULL -mt */
> >  		kfree(msgs);
> >  		return ERR_PTR(ret);
> >  	}
> > @@ -572,8 +608,17 @@
> >  	if (msgs) {
> >  		info->attr.mq_maxmsg = attr.mq_maxmsg;
> >  		info->attr.mq_msgsize = attr.mq_msgsize;
> > +		spin_lock(&mq_lock);
> > +		current->user->msg_queues -= (info->attr.mq_maxmsg 
> > +						* sizeof (struct msg_msg *) +
> > +						(info->attr.mq_maxmsg * 
> > +						info->attr.mq_msgsize));
> > +		if (current->user->msg_queues < 0)
> > +			current->user->msg_queues = 0;	
> 
> Oops, I think the subtraction is slightly wrong here.  Should be before
> the info->attr is updated, else you are actually carrying accounting for
> the default size (minus the actually allocated size).  Should be
> subtracting off the recently added default size.
> 
> New patch below (based on 2.6.6-rc3-bk).  Couple known issues are the
> possible mq_bytes caluclation overflow (not yet fixed in this patch),
> and setuid issue on signal side.  All other known issues have been
> addressed.

The setuid issue on signal side is pretty harmless though. It only affects the
users which are setuid() capable, and which do so with signal pending.

I see no other way to fix it than cache the user struct in "struct sigqueue". 
Thats not good news for performance (not sure how bad it would matter).

Again, thanks!