linux-kernel - Re: [PATCH V2] eventfd: change int to __u64 in eventfd

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: <4F8FC88C.7080603@gmail.com>
Date:	Thu, 19 Apr 2012 16:10:52 +0800
From:	Sha Zhengju <handai.szj@...il.com>
To:	Andrew Morton <akpm@...ux-foundation.org>
CC:	Sha Zhengju <handai.szj@...il.com>, linux-kernel@...r.kernel.org,
	davidel@...ilserver.org, avi@...hat.com
Subject: Re: [PATCH V2] eventfd: change int to __u64 in eventfd_signal()

On 04/19/2012 07:53 AM, Andrew Morton wrote:
> On Tue, 17 Apr 2012 11:44:36 +0800
> Sha Zhengju<handai.szj@...il.com>  wrote:
>
>> From: Sha Zhengju<handai.szj@...bao.com>
>>
>> eventfd_ctx->count is an __u64 counter which is allowed to reach ULLONG_MAX.
>> Now eventfd_write() add an __u64 value to "count", but kernel side
>> eventfd_signal() only add an int value to it. So make them consistent.
>>
>> ...
>>
>> --- a/fs/eventfd.c
>> +++ b/fs/eventfd.c
>> @@ -51,15 +51,13 @@ struct eventfd_ctx {
>>    *
>>    * -EINVAL    : The value of @n is negative.
>>    */
>> -int eventfd_signal(struct eventfd_ctx *ctx, int n)
>> +__u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
>>   {
>>   	unsigned long flags;
>>
>> -	if (n<  0)
>> -		return -EINVAL;
>>   	spin_lock_irqsave(&ctx->wqh.lock, flags);
>>   	if (ULLONG_MAX - ctx->count<  n)
>> -		n = (int) (ULLONG_MAX - ctx->count);
>> +		n = ULLONG_MAX - ctx->count;
>>   	ctx->count += n;
>>   	if (waitqueue_active(&ctx->wqh))
>>   		wake_up_locked_poll(&ctx->wqh, POLLIN);
> The comment needs updating:
>
> --- a/fs/eventfd.c~eventfd-change-int-to-__u64-in-eventfd_signal-fix
> +++ a/fs/eventfd.c
> @@ -46,10 +46,8 @@ struct eventfd_ctx {
>    * value, and we signal this as overflow condition by returining a POLLERR
>    * to poll(2).
>    *
> - * Returns @n in case of success, a non-negative number lower than @n in case
> - * of overflow, or the following error codes:
> - *
> - * -EINVAL    : The value of @n is negative.
> + * Returns the amount by which the counter was incrememnted.  This will be less
> + * than @n if the counter has overflowed.
>    */
>   __u64 eventfd_signal(struct eventfd_ctx *ctx, __u64 n)
>   {
>
> This doesn't seem a very useful return value.  Shouldn't it inform the
> user about overflow?  I guess the caller compares the return value to
> `n'.  Of course, no callers bother doing this :(
>
> What happens if the counter overflows?  It stops being updated.  What
> is the user-visible effect of that?
>

yeah... If callers check the return value, there is no difference to
return a subtracted value or an error code in case of overflow.
But even if the counter overflows, someone still will be wakeup
but get the incorrect number(depending on whether it cares the
value or not).
We can only inform about overflow but don't wake up waiters just
as eventfd_write() does.

> (It's presumably not an issue at present with a 64-bit counter, but
> might be a problem with your unexplained proposal of permitting
> userspace to add to the counter)
> .
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/