linux-kernel - Re: [Patch] Fix up return value from mq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4E85A074.6060201@xsintricity.com>
Date:	Fri, 30 Sep 2011 06:56:52 -0400
From:	Doug Ledford <dledford@...ntricity.com>
To:	Jiri Slaby <jslaby@...e.cz>
CC:	Andrew Morton <akpm@...ux-foundation.org>,
	linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [Patch] Fix up return value from mq_open()

On 9/30/2011 2:40 AM, Jiri Slaby wrote:
> On 09/30/2011 04:15 AM, Doug Ledford wrote:
>> Commit d40dcdb ipc/mqueue.c: fix mq_open() return value
>> changed the return value for mq_open() to return
>>
>> EMFILE - The process already has the maximum number of files and
>>                message queues open.
>>
>> instead of
>>
>> ENOMEM - Insufficient memory.
>>
>> While the portion of d40dcdb that introduced using ERR_PTR() to
>> propagate the condition were helpful, the change to the type of error
>> was not.  The specific test that was failing was a memory space test.
>> It can fail if there are too many message queues open and this one would
>> cause us to exceed the tasks counter for memory bytes capacity (aka, int
>> wrap) or it can fail if the total number of bytes already allocated plus
>> this allocation will exceed the user's RLIMIT for POSIX message queues.
> 
> Yes, then it should not definitely return ENOMEM, right? You exceeded a
> limit, not ran out of memory. At least for the latter case.

I would prefer another return myself, but there doesn't seem to be one.
 I had originally toyed with the idea of returning EPERM because you
don't have permissions to exceed the limit, but that's usually for file
permissions so I decided that would be confusing.  In the end, I stuck
with ENOMEM because that's what you would get if the ulimit for memory
was set low and you tried to exceed it.  In theory, ENOMEM means out of
memory in the system, in practice it's also used for memory consumption
in excess of limits.

>>  However, depending on the values passed in via an attribute struct,
>> this could happen on the 1st file or the 100th file. There is no direct
>> relationship to file count in this test.  There is only a direct
>> correlation to memory usage.  Therefore return the code to giving ENOMEM
>> instead of EMFILE.
> ...
>> commit b80e4058e2b4234fb33a9fd84b4ce288e8ba65cb
>> Author: Doug Ledford <dledford@...hat.com>
>> Date:   Thu Sep 29 19:02:47 2011 -0400
>>
>>     ipc/mqueue: make the error returns match the conditions
>>     
>>     We should not return -EMFILE when the problem is memory usage.
> 
> Sorry, I still don't udnerstand, how this test:
>   u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)
> is about running out of memory.

Type ulimit -a, you'll see that RLIMIT_MSGQUEUE is specifically for the
number of bytes of memory consumed by POSIX message queues as reported
by bash anyway.  That's memory.  And it's being compared against the
total amount of memory consumed in bytes after the allocation, not
against the number of queues created.

> I see only a test for exceeding some
> quota. Should this be changed, change it to something like ENOSPC.

Or fix the man page...

Your original quote about ENOMEM was from the man page, just update it
to read something like:

ENOMEM - Insufficient memory.  The application has tried to open a POSIX
message queue and there is either A) insufficient memory in the system
to create the queue or B) the application has exceeded the user specific
RLIMIT for the number of message queue bytes allocation to the
application or C) the application has run into the linux implementation
specific limit of 2GB of POSIX message queue bytes per application
across all open message queues.

>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
>> index 0474ddb..3e53604 100644
>> --- a/ipc/mqueue.c
>> +++ b/ipc/mqueue.c
>> @@ -165,7 +165,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb,
>>  		    u->mq_bytes + mq_bytes > task_rlimit(p, RLIMIT_MSGQUEUE)) {
>>  			spin_unlock(&mq_lock);
>>  			/* mqueue_evict_inode() releases info->messages */
>> -			ret = -EMFILE;
>> +			ret = -ENOMEM;
> 
> The code now looks like:
> int ret = -ENOMEM;
> ...
> if (sth)
>   ret = -ENOMEM;
> ...
> return ret;
> 
> which is not good.

I agree, but I left it because we hadn't heard from you yet and if it
needed to be set to something other than ENOMEM it would be easy to do
if the infrastructure for supporting it had not been ripped out already.

-- 
Doug Ledford <dledford@...ntricity.com>
              GPG KeyID: D30533DF

Download attachment "signature.asc" of type "application/pgp-signature" (261 bytes)