On Thu, Jun 16, 2022 at 9:41 AM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
>
> On Thu, Jun 16, 2022 at 8:57 AM Stanislav Fomichev <sdf@xxxxxxxxxx> wrote:
> > On Wed, Jun 15, 2022 at 6:36 PM Maciej Żenczykowski <maze@xxxxxxxxxx> wrote:
> > > I'm guessing this means the regression only affects 64-bit archs,
> > > where long = void* is 8 bytes > u32 of 4 bytes, but not 32-bit ones,
> > > where long = u32 = 4 bytes
> > >
> > > Unfortunately my dev machine's 32-bit build capability has somehow
> > > regressed again and I can't check this.
> >
> > Seems so, yes. But I'm actually not sure whether we should at all
> > treat it as a regression. There is a question of whether that EPERM is
> > UAPI or not. That's why we most likely haven't caught it in the
> > selftests; most of the time we only check that syscall has returned -1
> > and don't pay attention to the particular errno.
>
> EFAULT seems like a terrible error to return no matter what, it has a very clear
> 'memory read/write access violation' semantic (ie. if you'd done from
> userspace you'd get a SIGSEGV)
>
> I'm actually surprised to learn you return EFAULT on positive number...
> It should rather be some unique error code or EINVAL or something.
>
> I know someone will argue that (most/all) system calls can return EFAULT...
> But that's not actually true. From a userspace developer the expectation is
> they will not return EFAULT if you pass in memory you know is good.
>
> #include <sys/utsname.h>
> int main() {
> struct utsname uts;
> uname(&uts);
> }
>
> The above cannot EFAULT in spite of it being documented as the only
> error uname can report,
> because obviously the uts structure on the stack is valid memory.
>
> Maybe ENOSYS would at least make it obvious something is very weird.
I'd like to see less of the applications poking into errno and making
some decisions based on that. IMO, the only time where it makes sense
is EINTR/EAGAIN vs the rest; the rest should be logged. Having errnos
hard-coded in the tests is fine to make sure that the condition you're
testing against has been triggered; but still treating them as UAPI
might be too much, idk.
We had to add bpf_set_retval() because some of our and third party
libraries would upgrade to v6/v4 sockets only when they receive some
specific errno, which doesn't make a lot of sense to me.
