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] [thread-next>] [day] [month] [year] [list]

Message-Id: <20120418165332.3561032d.akpm@linux-foundation.org>
Date:	Wed, 18 Apr 2012 16:53:32 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Sha Zhengju <handai.szj@...il.com>
Cc:	linux-kernel@...r.kernel.org, davidel@...ilserver.org,
	avi@...hat.com, Sha Zhengju <handai.szj@...bao.com>
Subject: Re: [PATCH V2] eventfd: change int to __u64 in eventfd_signal()

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?

(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/