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    [W:0.027 / U:0.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site