From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Frysinger Subject: Re: [PATCH] hrtimer: make epoll_wait() use the hrtimer range feature Date: Wed, 24 Nov 2010 15:57:13 -0500 Message-ID: References: <1281307532-3235-1-git-send-email-shawn.bohrer@gmail.com> <20101124145247.GA2860@BohrerMBP.rgmadvisors.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Alexander Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org To: Shawn Bohrer Return-path: Received: from mail-wy0-f174.google.com ([74.125.82.174]:54518 "EHLO mail-wy0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755659Ab0KXU5f convert rfc822-to-8bit (ORCPT ); Wed, 24 Nov 2010 15:57:35 -0500 In-Reply-To: <20101124145247.GA2860@BohrerMBP.rgmadvisors.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, Nov 24, 2010 at 09:52, Shawn Bohrer wrote: > On Wed, Nov 24, 2010 at 03:33:02AM -0500, Mike Frysinger wrote: >> On Sun, Aug 8, 2010 at 18:45, Shawn Bohrer wrote: >> > @@ -1116,18 +1113,22 @@ static int ep_send_events(struct eventpoll= *ep, >> > =C2=A0static int ep_poll(struct eventpoll *ep, struct epoll_event = __user *events, >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 int= maxevents, long timeout) >> > =C2=A0{ >> > - =C2=A0 =C2=A0 =C2=A0 int res, eavail; >> > + =C2=A0 =C2=A0 =C2=A0 int res, eavail, timed_out =3D 0; >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0unsigned long flags; >> > - =C2=A0 =C2=A0 =C2=A0 long jtimeout; >> > + =C2=A0 =C2=A0 =C2=A0 long slack; >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0wait_queue_t wait; >> > - >> > - =C2=A0 =C2=A0 =C2=A0 /* >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0* Calculate the timeout by checking f= or the "infinite" value (-1) >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0* and the overflow condition. The pas= sed timeout is in milliseconds, >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0* that why (t * HZ) / 1000. >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ >> > - =C2=A0 =C2=A0 =C2=A0 jtimeout =3D (timeout < 0 || timeout >=3D E= P_MAX_MSTIMEO) ? >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MAX_SCHEDULE_TI= MEOUT : (timeout * HZ + 999) / 1000; >> > + =C2=A0 =C2=A0 =C2=A0 struct timespec end_time; >> > + =C2=A0 =C2=A0 =C2=A0 ktime_t expires, *to =3D NULL; >> > + >> > + =C2=A0 =C2=A0 =C2=A0 if (timeout > 0) { >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ktime_get_ts(&e= nd_time); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timespec_add_ns= (&end_time, (u64)timeout * NSEC_PER_MSEC); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 slack =3D estim= ate_accuracy(&end_time); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 to =3D &expires= ; >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 *to =3D timespe= c_to_ktime(end_time); >> > + =C2=A0 =C2=A0 =C2=A0 } else if (timeout =3D=3D 0) { >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timed_out =3D 1= ; >> > + =C2=A0 =C2=A0 =C2=A0 } >> > >> > =C2=A0retry: >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0spin_lock_irqsave(&ep->lock, flags); >> > @@ -1149,7 +1150,7 @@ retry: >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 * to TASK_INTERRUPTIBLE before doing the checks. >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 */ >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0set_current_state(TASK_INTERRUPTIBLE); >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (!list_empty(&ep->rdllist) || !jtimeout) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (!list_empty(&ep->rdllist) || timed_out) >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0break; >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0if (signal_pending(current)) { >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0res =3D -EINTR; >> > @@ -1157,7 +1158,9 @@ retry: >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0} >> > >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0spin_unlock_irqrestore(&ep->lock, flags); >> > - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 jtimeout =3D schedule_timeout(jtimeout); >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 if (!schedule_hrtimeout_range(to, slack, HRTIMER_MODE_ABS)) >> > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 timed_out =3D 1; >> > + >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0spin_lock_irqsave(&ep->lock, flags); >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0__remove_wa= it_queue(&ep->wq, &wait); >> >> this code introduces a warning: >> fs/eventpoll.c: In function =E2=80=98ep_poll=E2=80=99: >> fs/eventpoll.c:1119: warning: =E2=80=98slack=E2=80=99 may be used un= initialized in this function >> >> looks to me like you arent properly handling negative timeouts. >> certainly epoll_wait() passes the timeout value straight from >> userspace to ep_poll() without any error checking, so if userspace >> passes a negative timeout value, it looks like "slack" will be used >> uninitialized. > > If a negative timeout is passed in then 'to' remains NULL. =C2=A0When= 'to > is NULL schedule_hrtimeout_range() has an infinite timeout and the > 'slack' parameter is never used. =C2=A0So technically everything shou= ld be > fine here. ok, but that depends on an external function never changing behavior and makes changing the API pretty hard since all callers must be closely analyzed > Of course it would be safest and best to simply initialize slack to 0= =2E > I can send a patch this evening with the fix. thanks ! -mike -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel= " in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html