Re: [Patch 2/4] ipc/mqueue: correct mq_attr_ok test

From: Doug Ledford
Date: Tue May 01 2012 - 15:38:06 EST


On 05/01/2012 03:34 PM, Andrew Morton wrote:
> On Tue, 1 May 2012 13:50:53 -0400
> Doug Ledford <dledford@xxxxxxxxxx> wrote:
>
>> While working on the other parts of the mqueue stuff, I noticed that
>> the calculation for overflow in mq_attr_ok didn't actually match
>> reality (this is especially true since my last patch which changed
>> how we account memory slightly).
>
> Please cc Manfred on mqueue things? He still watches ;)
>
>> In particular, we used to test for overflow using:
>> msgs * msgsize + msgs * sizeof(struct msg_msg *)
>>
>> That was never really correct because each message we allocate via
>> load_msg() is actually a struct msg_msg followed by the data for
>> the message (and if struct msg_msg + data exceeds PAGE_SIZE we end
>> up allocating struct msg_msgseg structs too, but accounting for them
>> would get really tedious, so let's ignore those...they're only a
>> pointer in size anyway). This patch updates the calculation to be
>> more accurate in regards to maximum possible memory consumption by the
>> mqueue.
>>
>> ...
>>
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>>
>> ...
>>
>> @@ -684,8 +686,11 @@ static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
>> /* check for overflow */
>> if (attr->mq_msgsize > ULONG_MAX/attr->mq_maxmsg)
>> return 0;
>> - if ((unsigned long)(attr->mq_maxmsg * (attr->mq_msgsize
>> - + sizeof (struct msg_msg *))) <
>> + mq_treesize = attr->mq_maxmsg * sizeof(struct msg_msg) +
>> + min_t(unsigned int, attr->mq_maxmsg, MQ_PRIO_MAX) *
>> + sizeof(struct posix_msg_tree_node);
>> + if ((unsigned long)(attr->mq_maxmsg * attr->mq_msgsize +
>> + mq_treesize) <
>> (unsigned long)(attr->mq_maxmsg * attr->mq_msgsize))
>> return 0;
>> return 1;
>
> That's a bit of a mouthful. Does this look OK?
>
> --- a/ipc/mqueue.c~ipc-mqueue-correct-mq_attr_ok-test-fix
> +++ a/ipc/mqueue.c
> @@ -672,7 +672,8 @@ static void remove_notification(struct m
> static int mq_attr_ok(struct ipc_namespace *ipc_ns, struct mq_attr *attr)
> {
> int mq_treesize;
> -
> + unsigned long total_size;
> +
> if (attr->mq_maxmsg <= 0 || attr->mq_msgsize <= 0)
> return 0;
> if (capable(CAP_SYS_RESOURCE)) {
> @@ -690,9 +691,8 @@ static int mq_attr_ok(struct ipc_namespa
> mq_treesize = attr->mq_maxmsg * sizeof(struct msg_msg) +
> min_t(unsigned int, attr->mq_maxmsg, MQ_PRIO_MAX) *
> sizeof(struct posix_msg_tree_node);
> - if ((unsigned long)(attr->mq_maxmsg * attr->mq_msgsize +
> - mq_treesize) <
> - (unsigned long)(attr->mq_maxmsg * attr->mq_msgsize))
> + total_size = attr->mq_maxmsg * attr->mq_msgsize;
> + if (total_size + mq_treesize < total_size)
> return 0;
> return 1;
> }

Sure, looks fine to me and should preserve the wrap around test behavior.


--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: 0E572FDD
http://people.redhat.com/dledford


Attachment: signature.asc
Description: OpenPGP digital signature