SONiC buildimage ARM arch support

[antony-rheneus]

docker/sonic-slave/Makefile
ARM Architecture support in SONIC

make configure platform=[ASIC_VENDOR_ARCH] PLATFORM_ARCH=[ARM_ARCH]
SONIC_ARCH: default amd64
armhf - arm32bit
arm64 - arm64bit

[msftclas]

All CLA requirements met.

[lguohan]

the default x64 build all fails, is that expected?

[qiluo-msft]

SONiC has no indent to customize the operation environment unless truly necessary. Please remove .vimrc #Closed

---

Refers to: dockers/docker-base-arm64/root/.vimrc:3 in 5d35834. [](commit_id = 5d35834, deletion_comment = False)

[qiluo-msft]

And this is only for root user, not a general solution.

---

In reply to: 500067714 [](ancestors = 500067714)

---

Refers to: dockers/docker-base-arm64/root/.vimrc:3 in 5d35834. [](commit_id = 5d35834, deletion_comment = False)

[qiluo-msft]

4.4.8 [](start = 10, length = 5)

amd64 is already using 4.9.0. Could you use the same or newer version? #Pending

[antony-rheneus]

For Marvell ARM platform we are in process of analysing kernel migration to latest version.
Since we have tested with 4.4.8, I have retained it

[qiluo-msft]

I see your concern. Still I prefer the same version with all other platforms. If we introduce different ABI for master branch, some other component may have issues on this platform.

---

In reply to: 292588057 [](ancestors = 292588057)

[lguohan]

some sonic features depends on the kernel especially vrf feature, what's your plan to bring kernel to 4.9.0?

[antony-rheneus]

We have image running with kernel 4.14.76 and is ready now.
Do you still insist on 4.9 version, I have not started working on migrating to 4.9 kernel version.

[lguohan]

yes, what I am expecting is that we have your arm kernel compile from https://github.com/Azure/sonic-linux-kernel

[antony-rheneus]

Updated kernel version 4.9.168 as debian package.
In parallel working on to compile kernel from sonic-linux-kernel too.

[antony-rheneus]

the default x64 build all fails, is that expected?

[antony-rheneus]

the default x64 build all fails, is that expected?

Let me check again, I verified x86_64 before pushing.

[antony-rheneus]

since you added sonic-slave/dockerfile.j2, and sonic-slave-stretch/dockerfile.j2, can you remove the two dockerfile?

Cannot remove the dockerfiles, as jenkins build machine doesn't have jinja2

[lguohan]

retest vs please

[lguohan]

please remove *.pyc files

[lguohan]

I suggest you to use a new PR to add your sku. focus this PR on introduce new build arch support. The PR is already very large. adding more commits will delaying merging this PR.

[antony-rheneus]

I suggest you to use a new PR to add your sku. focus this PR on introduce new build arch support. The PR is already very large. adding more commits will delaying merging this PR.

Removed hwsku changes

[antony-rheneus]

please remove *.pyc files

removed hwsku directory where *.pyc were present

[jleveque]

@antony-rheneus: Can you please explain why you made this change? Subtracting 1 from the size of the buffer ensures that the copied string will be NULL-terminated. This change could potentially result in a non-terminated buffer. It seems like there is now the potential for a crash with this change rather than without it.

[antony-rheneus]

Excerpt from Man page strncpy:-
The strncpy() function is similar, except that at most n bytes of src are copied. Warning: If there is no null byte among the first n bytes of src, the string placed in dest will not be null-terminated.

Issue in Code:-
Max Interface name size is 16 bytes.
info->name is NOT initialized/memset to 0 (NULL).
If source string "name" has 15 bytes like in case of "PortChannel0001", then earlier code will copy only 15 bytes (max_size - 1). So it would not copy the Last NULL character from the 16th byte of source string "name"
This leads to seg fault/crash

There is a check above this line that Source string "name" will not go beyond max size 16 bytes.
And Source string should be NULL terminated by the caller.
Now this strncpy line should ensure to add NULL terminator at the last byte.

Your concern of Non-Terminated NULL buffer would arise only if source string "name" is NOT null terminated or is more than 15 bytes. But this would not happen as the below lines would catch it and return Error.

 546                         len = p - name;
 547                         if (len >= sizeof(info->name)) {

[jleveque]

@antony-rheneus: Thanks for the quick reply. I now understand your reasoning. However, a few lines below this, name is also copied into tmp.ifr_name in a similar fashion. However, in this case, the buffer is initialized to 0 beforehand:

    memset(&tmp, 0, sizeof(tmp));
    strncpy(tmp.ifr_name, name, sizeof(tmp.ifr_name) - 1);

Therefore, with your patch, these two strncpy operations differ, and at first glance it is not clear why. Thus, I think it makes more sense to keep these copy operations identical in behavior. So I suggest that we instead explicitly memset the buffer info->name to 0 before doing the strncpy.

    memset(&info->name, 0, sizeof(info->name));
    strncpy(info->name, name, sizeof(info->name) - 1);

I think this is the most easily understood code, and an explicit memset of info->name couldn't hurt, especially because they unnecessarily define it to be one byte longer than IFNAMSIZ.

struct iface_info {
    char name[IF_NAMESIZE+1];   /* name of the interface, e.g. "bge0" */
    struct sockaddr_storage addr;   /* address information */
    struct sockaddr_storage netmask;    /* netmask information */
    isc_uint64_t flags;     /* interface flags, e.g. IFF_LOOPBACK */
};

What are your thoughts on my proposal?

[qiluo-msft]

@antony-rheneus In future, please don't include changes not related to main topic into a big PR like this. I approved at iteration 33, but you still made significant code changes after that. My approval is not applied to anything after that iteration.

I agree this strncpy is a valid bug fix. I notice another occurrence, please check whether it is a bug.

		strncpy(info->name, name, sizeof(info->name) - 1);

#ifdef SKIP_DUMMY_INTERFACES
	} while (strncmp(info->name, "dummy", 5) == 0);
#else
	} while (0);
#endif

[jleveque]

@qiluo-msft: As I mentioned above, I believe the proper fix is to leave the strncpy calls untouched and memset the buffer to 0 before each call.

[antony-rheneus]

@jleveque ,
From which file you referred iface_info? I could see below in SONIC dhcp submodule for name[IFNAMSIZ]

src/isc-dhcp/isc-dhcp/common/discover.c
 399 /*
 400  * Structure used to return information about a specific interface.
 401  */
 402 struct iface_info {
 403         char name[IFNAMSIZ];            /* name of the interface, e.g. "eth0" */
 404         struct sockaddr_storage addr;   /* address information */
 405     struct sockaddr_storage netmask;    /* netmask information */
 406         isc_uint64_t flags;             /* interface flags, e.g. IFF_LOOPBACK */
 407 };
 408

I agree with your proposal, by doing memset before strncpy.
However using strncpy in right way will avoid cpu cycles of memset operation.

[jleveque]

@antony-rheneus: I mistakenly quoted from the Solaris section at lines 227-230. The definition you quoted is the Linux definition, so the buffer is only IFNAMSIZ is length.

I'm not very concerned about a few extra CPU cycles here, as interface discovery only occurs once when the relay agent starts.

Also, I respectfully disagree that your solution is the "right way" to use strncpy. This is debatable. I personally think the proper way to use strncpy is to explicitly write a NUL char after the strncpy call. This not only ensures there will be no overflow, but it is easy to read and understand what is taking place. In the case of this code, a memset first will accomplish the same thing, but if I wrote the code myself, I would have explicitly written a NUL char after the strncpy call.

[antony-rheneus]

I agree.

[jleveque]

FYI, The following upstream patch introduced a memset call before the strncpy call, much like I had suggested earlier in these comments: https://gitlab.isc.org/isc-projects/dhcp/-/commit/b7bfb2c74821ad6055c349dd2eddf2c749578762. This patch was included in the 4.4.0 release of isc-dhcp.

We are upgrading the dhcp_relay Docker container to be based off Debian Buster, for which the native version of isc-dhcp is currently 4.4.1-2. Therefore, I have requested this patch be removed as part of the upgrade, as it is no longer necessary.