lkml.org 
[lkml]   [2004]   [May]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] per-user signal pending and message queue limits
* Marcelo Tosatti (marcelo.tosatti@cyclades.com) wrote:
> > 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?

Yes, exactly.

> 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.

I think so. I've no idea what a good max is, but last night I simply
added some overflow detection to handle this.

> > > + 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.

Yes. It's part of why I left it for now.

> 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).

My thoughts exactly.

thanks,
-chris
--
Linux Security Modules http://lsm.immunix.org http://lsm.bkbits.net
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:02    [from the cache]
©2003-2011 Jasper Spaans