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

> 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!
-
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    [W:0.084 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site