VDE fixes

[jguillaumes]

Hello,

This are the small changes I made to support long VDE interface names.

[larsbrinkhoff]

See problem statement in #25.

[larsbrinkhoff]

Hello @jguillaumes,

I'd like to clean this up. May I force push to your master branch?

[jguillaumes]

Sure! I'm not sure of the protocol you follow here... should I merge the changes myself or are you going to do it for me?

[larsbrinkhoff]

I'm not sure what the protocol is. I consider @Rhialto the primary maintainer, so I suppose he should sign off on changes.

[larsbrinkhoff]

I have a question: Why not just increase IFNAM_LEN?

[larsbrinkhoff]

I rearranged the changes so that there are just two commits. The first is the VDE changes, and the second to update gitignore. The end result is identical to the original pull request, but you may want to check that it still works just in case.

[Rhialto]

On 28 November 2017 09:02:35 CET, Lars Brinkhoff notifications@github.com wrote:

I have a question: Why not just increase IFNAM_LEN?

I haven't had time yet to look at it but i imagined that might be an aspect.

[jguillaumes]

I considered it. Then I saw the length is specifically checked to be within limits. That check could be to prevent a buffer overflow, but it also could be to prevent some problem with the underlying libraries os OSs. So I went cautious and decided to use a different area specifically for VDE pathnames.

The problem is you can’t be sure the alternative works without testing against ALL the platforms. The kludge I made will work without breaking anything (hopefully).

[Rhialto]

I've looked at it a bit and here are my thoughts: first, before I forget: DPNI20_VERSION in dpni20.h should be increased, so that not two incompatible versions can try to communicate together. I found the repeated checking for "vde" rather ugly, so I'm going to use the following comment from the same header as an argument for the rest:

#define IFNAM_LEN 16 /* at least IFNAMSIZ! */

Apparently it is already foreseen that IFNAM_LEN might be larger than IFNAMSIZ. So I see no reason there to avoid actually using that approach. It seems that in most places where the string is used, its length is already checked against IFNAMSIZ but it won't hurt to double-check that this is indeed the case. A somewhat ugly detail might be that IFNAM_LEN is defined in multiple places; in dpimp.h, dpni20.h, enaddr.h. The last one is a bit ugly. Hopefully the definitions won't conflict. Perhaps this is something that should be (have been) fixed too.

[larsbrinkhoff]

@jguillaumes Will you try increasing IFNAM_LEN instead?

[jguillaumes]

I tried it yesterday, just brute-forced it changing the sizes and adding <sys/syslimits.h> where required. It doesn’t work: the software tries to open a bpf device instead of a vde one. My guess is memory gets overwritten at some point. It needs more research and I’ll give it a try when I have enough time to spend at it.

[larsbrinkhoff]

Ok, thanks!

[Rhialto]

It looks like there are some dependencies missing in the Makefile system. I just changed dpni20.h and only dpni20.c got recompiled. Of course the main emulator should rebuild as well.

[Rhialto]

How about something like https://github.com/PDP-10/klh10/compare/ifnamlen ? I haven't tried it beyond compiling, since I don't use VDE myself.

[jguillaumes]

It looks there is still a length test:

KLH10# go
Starting KN10 at loc 040000...

BOOT V11.0(315)

BOOT>

[BOOT: Loading] [OK]

[TOPS20 mounted]
Testing Panda Distribution, PANDA TOPS-20 Monitor 7.1(21733)-4 Internet: Loading host names [OK]

System restarting, wait...
Date and time is: Wednesday, 29-November-2017 9:52PM
Why reload? n
Run CHECKD? n
 DDMP: Started
[KNILDR: Loading microcode version 1(172) into Ethernet channel 0]

[dpni20: Warning - cannot set high priority - Permission denied]

[dpni20: *** Must usually run as superuser; networking may fail! ***]
[dpni20: Warning - cannot lock memory]

[dpni20: Fatal error: interface name "/private/tmp/vde.ctl" too long - max 16 - Function not implemented]
29-Nov-2017 21:52:31 ***BUGCHK KNIINF*** PHYKNI - NIA20 initialization timed out  Job: 0, User: OPERATOR, Data: 400007000036

SYSJOB 7A(88)-4 started at 29-Nov-2017 2152
29-Nov-2017 21:52:36 ***BUGCHK IPDWNS*** Datagram was not sent  Job: 0, User: OPERATOR, Data: 36, 2
29-Nov-2017 21:52:36 ***BUGCHK KNIRLF*** PHYKNI - NIA20 Reload Failed  Job: 0, User: OPERATOR, Data: 10
SJ  0: @LOGIN OPERATOR

This is the length test we are failing, inside net_init(), around the line 470:

  /* Ensure network device name, if specified, isn't too long */
    if (dpni->dpni_ifnam[0] && (strlen(dpni->dpni_ifnam)
        >= sizeof(ifr.ifr_name))) {
    esfatal(0, "interface name \"%s\" too long - max %d",
        dpni->dpni_ifnam, (int)sizeof(ifr.ifr_name));
    }

This test is being made against the allocated size for ifr.ifr_name. ift is of type ifreq, which is a system-defined structure declared in net/if.h. So the only way to pass it will be to actually NOT doing the comparison for ifmeth=vde or changing the sizeof() by a PATH_MAX. None of the solutions is clean nor nice.

[jguillaumes]

Just checked it, changing those lines by:

   /* Ensure network device name, if specified, isn't too long */
    /* Don't do this test for VDE devices, which can be longer */
    if (strncmp(dpni->dpni_ifmeth, "vde", sizeof(dpni->dpni_ifmeth)-1)) {
        if (dpni->dpni_ifnam[0] && (strlen(dpni->dpni_ifnam)
                                    >= sizeof(ifr.ifr_name))) {
            esfatal(0, "interface name \"%s\" too long - max %d",
                    dpni->dpni_ifnam, (int)sizeof(ifr.ifr_name));
        }
    }

Now it seems to work. Although I know you don't like the vde-specific test, doing it this way lets us to keep a sanity test for real, non-vde interface names.

[Rhialto]

I see. Well, then this check can be removed entirely, I think. I put checks for the length in other locations, which would probably cover all non-VDE cases.

[Rhialto]

I have added my suggestion above to the ifnamlen branch. Does that seem ok?

[jguillaumes]

On 10 Dec 2017, at 15:44, Rhialto The M. notifications@github.com wrote:

I have added my suggestion above to the ifnamlen branch. Does that seem ok?

It looks OK and seems to work.

[jguillaumes]

@Rhialto , when you merge this to master, please remember to also "fix" the connection to VDE so klh10 identifies itself as "klh10" instead of "simh". The only consequence of this that I can recall is it shows itself as "simh" when you query the current connections from vde, and it's quite ugly ;)

[Rhialto]

Duh, of course... I merged it already, so I'll just do that next. (I didn't start it on the branch and then re-merge it, because it's not really a topic that belongs to that branch).