[jakub@redhat.com: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/pppd: p

[jakub@redhat.com: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/pppd: p

[Bill Nottingham]

Back long ago, I reported a problem with PPP on ia64:

 http://www.gelato.unsw.edu.au/linux-ia64/0105/1641.html

We added the workaround in our tree for earlier releases,
and promptly forgot about it; however, someone reported that
RHEL 3 has the same issue (as we took the workaround out.)

Jakub noticed the following - does this explain the problem?

Bill

----- Forwarded message from Jakub Jelinek <jakub@redhat.com> -----

From: Jakub Jelinek <jakub@redhat.com>
To: notting@redhat.com
Subject: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/pppd: put_user/copy_to_user]
Reply-To: Jakub Jelinek <jakub@redhat.com>
User-Agent: Mutt/1.4.1i
X-Reply-To: Jakub Jelinek <jakub@redhat.com>
Date: Fri, 12 Dec 2003 07:32:59 -0500

On Thu, Dec 11, 2003 at 09:16:50PM -0800, David S. Miller wrote:
> On Thu, 11 Dec 2003 22:58:48 -0500
> Bill Nottingham <notting@redhat.com> wrote:
> 
> > According to Intel, this patch is *still* needed to fix ppp on
> > ia64 in RHEL3. We should probably verify this, although I'm
> > still at a loss as to why it works.
> 
> Either a compiler problem or their put_user() implementation
> is busted.

The latter I guess.
At least linux-2.4.22-1.2108.nptl I have unpacked on my box
has in asm-ia64/uaccess.h:

#define put_user(x,ptr) __put_user_check((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr)),get_fs())

#define __put_user_check(x,ptr,size,segment)                    \
({                                                              \
        register long __pu_err asm ("r8") = -EFAULT;            \
        __typeof__(*(ptr)) *__pu_addr = (ptr);                  \
        if (__access_ok((long)__pu_addr,size,segment)) {        \
                __pu_err = 0;                                   \
                switch (size) {                                 \
                  case 1: __put_user_8(x,__pu_addr); break;     \
                  case 2: __put_user_16(x,__pu_addr); break;    \
                  case 4: __put_user_32(x,__pu_addr); break;    \
                  case 8: __put_user_64(x,__pu_addr); break;    \
                  default: __put_user_unknown(); break;         \
                }                                               \
        }                                                       \
        __pu_err;                                               \
})

#define __put_user_32(x,addr)                                                           \
        asm volatile (                                                                  \
                "\n"_LL"\tst4 %1=%r2%P1\t// %0 gets overwritten by exception handler\n" \
                "\t.xdata4 \"__ex_table\", @gprel(1b), @gprel(1f)\n"                    \
                _LL                                                                     \
                : "=r"(__pu_err) : "m"(__m(addr)), "rO"(x), "0"(__pu_err))

But, a function call clobbers r8.
Guess a
__typeof(x) __x = (x);
before __pu_err and
s/x/__x/ is needed.
Probably __pu_addr decl should be moved before __pu_err is initialized too,
otherwise
extern int *foo ();
put_user (0, foo ());
might not work properly.

	Jakub



----- End forwarded message -----

Re: [jakub@redhat.com: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/ppp

[Bjorn Helgaas]

__put_user_check in both the 2.4 and 2.5 BK trees looks like the
one quoted below, so evidently the problem has not been fixed.
If you have a proposed fix, a patch and a test case would be
useful.

On Friday 12 December 2003 8:37 am, Bill Nottingham wrote:
> Back long ago, I reported a problem with PPP on ia64:
> 
>  http://www.gelato.unsw.edu.au/linux-ia64/0105/1641.html
> 
> We added the workaround in our tree for earlier releases,
> and promptly forgot about it; however, someone reported that
> RHEL 3 has the same issue (as we took the workaround out.)
> 
> Jakub noticed the following - does this explain the problem?
> 
> Bill
> 
> ----- Forwarded message from Jakub Jelinek <jakub@redhat.com> -----
> 
> From: Jakub Jelinek <jakub@redhat.com>
> To: notting@redhat.com
> Subject: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/pppd: put_user/copy_to_user]
> Reply-To: Jakub Jelinek <jakub@redhat.com>
> User-Agent: Mutt/1.4.1i
> X-Reply-To: Jakub Jelinek <jakub@redhat.com>
> Date: Fri, 12 Dec 2003 07:32:59 -0500
> 
> On Thu, Dec 11, 2003 at 09:16:50PM -0800, David S. Miller wrote:
> > On Thu, 11 Dec 2003 22:58:48 -0500
> > Bill Nottingham <notting@redhat.com> wrote:
> > 
> > > According to Intel, this patch is *still* needed to fix ppp on
> > > ia64 in RHEL3. We should probably verify this, although I'm
> > > still at a loss as to why it works.
> > 
> > Either a compiler problem or their put_user() implementation
> > is busted.
> 
> The latter I guess.
> At least linux-2.4.22-1.2108.nptl I have unpacked on my box
> has in asm-ia64/uaccess.h:
> 
> #define put_user(x,ptr) __put_user_check((__typeof__(*(ptr)))(x),(ptr),sizeof(*(ptr)),get_fs())
> 
> #define __put_user_check(x,ptr,size,segment)                    \
> ({                                                              \
>         register long __pu_err asm ("r8") = -EFAULT;            \
>         __typeof__(*(ptr)) *__pu_addr = (ptr);                  \
>         if (__access_ok((long)__pu_addr,size,segment)) {        \
>                 __pu_err = 0;                                   \
>                 switch (size) {                                 \
>                   case 1: __put_user_8(x,__pu_addr); break;     \
>                   case 2: __put_user_16(x,__pu_addr); break;    \
>                   case 4: __put_user_32(x,__pu_addr); break;    \
>                   case 8: __put_user_64(x,__pu_addr); break;    \
>                   default: __put_user_unknown(); break;         \
>                 }                                               \
>         }                                                       \
>         __pu_err;                                               \
> })
> 
> #define __put_user_32(x,addr)                                                           \
>         asm volatile (                                                                  \
>                 "\n"_LL"\tst4 %1=%r2%P1\t// %0 gets overwritten by exception handler\n" \
>                 "\t.xdata4 \"__ex_table\", @gprel(1b), @gprel(1f)\n"                    \
>                 _LL                                                                     \
>                 : "=r"(__pu_err) : "m"(__m(addr)), "rO"(x), "0"(__pu_err))
> 
> But, a function call clobbers r8.
> Guess a
> __typeof(x) __x = (x);
> before __pu_err and
> s/x/__x/ is needed.
> Probably __pu_addr decl should be moved before __pu_err is initialized too,
> otherwise
> extern int *foo ();
> put_user (0, foo ());
> might not work properly.
> 
> 	Jakub
> 
> 
> 
> ----- End forwarded message -----

Re: [jakub@redhat.com: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/ppp

[David Mosberger]

>>>>> On Fri, 12 Dec 2003 10:37:16 -0500, Bill Nottingham <notting@redhat.com> said:

  Bill> Back long ago, I reported a problem with PPP on ia64:
  Bill> http://www.gelato.unsw.edu.au/linux-ia64/0105/1641.html

  Bill> We added the workaround in our tree for earlier releases, and
  Bill> promptly forgot about it; however, someone reported that RHEL
  Bill> 3 has the same issue (as we took the workaround out.)

  Bill> Jakub noticed the following - does this explain the problem?

Yes, it sure does.  Argh, passing functions to get/put-user macro
arguments.  I suppose that's legal... ;-(

  Bill> But, a function call clobbers r8.  Guess a __typeof(x) __x   Bill> (x); before __pu_err and s/x/__x/ is needed.  Probably
  Bill> __pu_addr decl should be moved before __pu_err is initialized
  Bill> too, otherwise extern int *foo (); put_user (0, foo ()); might
  Bill> not work properly.

Yeah, we need to be careful not to evalute any macro arguments while
using the special register-usage convention needed for the exception
handlers.  It affects get_user(), too.  Let me work on a patch.

	--david

Re: [jakub@redhat.com: Re: [brian.j.vandecoevering@intel.com: RE: [Linux-ia64] problems with ppp/ppp

[David Mosberger]

>>>>> On Fri, 12 Dec 2003 10:37:16 -0500, Bill Nottingham <notting@redhat.com> said:

  Bill> Back long ago, I reported a problem with PPP on ia64:
  Bill> http://www.gelato.unsw.edu.au/linux-ia64/0105/1641.html

  Bill> We added the workaround in our tree for earlier releases, and
  Bill> promptly forgot about it; however, someone reported that RHEL
  Bill> 3 has the same issue (as we took the workaround out.)

  Bill> Jakub noticed the following - does this explain the problem?

OK, I fixed this now for 2.6:

  http://lia64.bkbits.net:8080/to-linus-2.5/cset@1.1504

That file really needed some cleanup.  Apart from fixing this
particular issue, I also consolidated the GCC vs. Intel macros so they
make more sense and so there is much less code-duplication.

Also, I did a fairly careful analysis of the impact of this bug.
Basically, I built a kernel with "allyesconfig" and tweaked the
uaccess macros to emit their arguments to a special string section,
which I then examined for function calls.  Since the inspection was
manual and there are _tons_ of get_user/put_user calls in the kernel,
it's possible I missed a few function calls, but it shouldn't be off
too far.  Fortunately, the result is that only 28 (out of hundreds) of
get_user/put_user invocations have real (non-inlined) function-calls
in their arguments, 18 are for PPP, 1 for PCMCIA (not an issue for
ia64), 4 for the OSS sound driver and 3 for some watchdog timers which
most likely are not supported on ia64 anyhow.  read_port() in the
/dev/mem driver also has this problem, but this function is defined
only if CONFIG_ISA is on, so we're OK.  The only new issue that came
to light is that the TIOCOUTQ ioctl also has this problem, so I'd
expect spurious failures for that ioctl().

In summary, only PPP and TIOCOUTQ seem to have been affected by this
bug, which probably explains why the bug remained in hiding for so
long.

Anyhow, I included a summary of the problematic calls below for
reference.

	--david

drivers/char/mem.c:321:__put_user: inb(i)
drivers/char/tty_ioctl.c:465:put_user: tty->driver->chars_in_buffer ? tty->driver->chars_in_buffer(tty) : 0
drivers/char/watchdog/wdt.c:342:put_user: wdt_status()
drivers/char/watchdog/wdt.c:342:put_user: wdt_status()
drivers/char/watchdog/wdt_pci.c:327:put_user: wdtpci_status()
drivers/net/ppp_async.c:264:put_user: ppp_channel_index(&ap->chan)
drivers/net/ppp_async.c:264:put_user: ppp_channel_index(&ap->chan)
drivers/net/ppp_async.c:274:put_user: ppp_unit_number(&ap->chan)
drivers/net/ppp_async.c:274:put_user: ppp_unit_number(&ap->chan)
drivers/net/ppp_synctty.c:317:put_user: ppp_channel_index(&ap->chan)
drivers/net/ppp_synctty.c:317:put_user: ppp_channel_index(&ap->chan)
drivers/net/ppp_synctty.c:327:put_user: ppp_unit_number(&ap->chan)
drivers/net/ppp_synctty.c:327:put_user: ppp_unit_number(&ap->chan)
drivers/pcmcia/ds.c:589:put_user: get_queued_event(user)
net/atm/pppoatm.c:338:put_user: ppp_channel_index(&atmvcc_to_pvcc(atmvcc)-> chan)
net/atm/pppoatm.c:338:put_user: ppp_channel_index(&atmvcc_to_pvcc(atmvcc)-> chan)
net/atm/pppoatm.c:341:put_user: ppp_unit_number(&atmvcc_to_pvcc(atmvcc)-> chan)
net/atm/pppoatm.c:341:put_user: ppp_unit_number(&atmvcc_to_pvcc(atmvcc)-> chan)
net/irda/irnet/irnet_ppp.c:668:put_user: ppp_channel_index(&ap->chan)
net/irda/irnet/irnet_ppp.c:668:put_user: ppp_channel_index(&ap->chan)
net/irda/irnet/irnet_ppp.c:668:put_user: ppp_channel_index(&ap->chan)
net/irda/irnet/irnet_ppp.c:676:put_user: ppp_unit_number(&ap->chan)
net/irda/irnet/irnet_ppp.c:676:put_user: ppp_unit_number(&ap->chan)
net/irda/irnet/irnet_ppp.c:676:put_user: ppp_unit_number(&ap->chan)
sound/core/seq/oss/seq_oss_ioctl.c:89:put_user: snd_seq_oss_writeq_get_free_size(dp->writeq)
sound/core/seq/oss/seq_oss_ioctl.c:89:put_user: snd_seq_oss_writeq_get_free_size(dp->writeq)
sound/core/seq/oss/seq_oss_ioctl.c:89:put_user: snd_seq_oss_writeq_get_free_size(dp->writeq)
sound/core/seq/oss/seq_oss_ioctl.c:89:put_user: snd_seq_oss_writeq_get_free_size(dp->writeq)