Fixed EINVAL in systemd-udevd processing of speed/duplex in valid .li…

[bajhn]

…nk files.

Including BitsPerSecond or Duplex values in .link files did not work when
set_slinksettings was called because the routine was not copying the base
parameters to the structure given to ioctl. As a result, EINVAL was always
reported, and no change occurred on the Ethernet device.

[yuwata]

It is not necessary to clear whole area. Just link_mode_data is sufficient, right?

[yuwata]

It may be still overkill. I do not know whether the space needs to clear or not, but if it is necessary, then clear the rest of space after memcpy(&ecmd.link_mode_data[offset], u->link_modes.lp_advertising, 4 * ecmd.req.link_mode_masks_nwords);.

[bajhn]

My rationale was that the previous code seemed to be zeroing the entire block:

static int set_slinksettings(int *fd, struct ifreq *ifr, const struct ethtool_link_usettings *u) {
        struct {
                struct ethtool_link_settings req;
                __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
        } ecmd = {
                .req.cmd = ETHTOOL_SLINKSETTINGS,
        };

Without that initialization, I believe ecmd comes into being with whatever's in the stack and would need to be zeroed. On my target, the function is being executed once for each link -- when the device is enabled. I haven't looked at assembly instructions generated by a compiler in over 20 years, and I'm not that good a coder, so feel free to make any optimizations you think necessary. The critical thing is that the base parameters weren't being copied, and they need to be. Thank you for your help!

[poettering]

Please initialize ecmd to zero during declaration, the compiler usually generates better code then. Hence, please change the declaration of the variable to struct { … } ecmd = {}; and drop the memset() here

[bajhn]

This is pushed in ac38698. Thanks!

[poettering]

Please initialize ecmd to zero during declaration, the compiler usually generates better code then. Hence, please change the declaration of the variable to struct { … } ecmd = {}; and drop the memset() here

[poettering]

Hmm, the u->base and ecmd.req should have the same type, no? If so, please just use normal assignment, not memcpy(). i.e.:

ecmd.req = u->base;

[bajhn]

I get your first change to eliminate the memset. Regarding the memcpy in 484, ecmd.req and u->base are struct ethtool_link_settings, not pointers to one. Does C now allow wholesale assignment of struct contents?

Looking at the comparable function from ethtool.c, they're doing a memcpy also, and not being abreast of the latest changes in C, I felt I was safe:

static int
do_ioctl_slinksettings(struct cmd_context *ctx,
                       const struct ethtool_link_usettings *link_usettings)
{
        struct {
                struct ethtool_link_settings req;
                __u32 link_mode_data[3 * ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NU32];
        } ecmd;
        unsigned int u32_offs;

        /* refuse to send ETHTOOL_SLINKSETTINGS ioctl if
         * link_usettings was retrieved with ETHTOOL_GSET
         */
        if (link_usettings->base.cmd != ETHTOOL_GLINKSETTINGS)
                return -1;

        /* refuse to send ETHTOOL_SLINKSETTINGS ioctl if deprecated fields
         * were set
         */
        if (link_usettings->deprecated.transceiver)
                return -1;

        if (link_usettings->base.link_mode_masks_nwords <= 0)
                return -1;

        memcpy(&ecmd.req, &link_usettings->base, sizeof(ecmd.req));
        ecmd.req.cmd = ETHTOOL_SLINKSETTINGS;
    . . .

I'm going to try pushing the first change. Please bear with me, as this is my first experience with Github and pull requests.

[poettering]

yes, C has supported assigning full struct since about always. memcpy() is not needed

[keszybz]

Indeed, I was sure this doesn't work. Glad to see that it does. So both set_glinksettings and get_glinksettings can be adjusted.

[poettering]

comparison of structs is not allowed in C, because the padding in structs makes this useless in the general case. But assignment of structs has always been fine

[poettering]

Note that I included a fix-up for this in #7816

[bajhn]

It looks to me like the Fedora Rawhide checks failed on some externality. Will they be re-run? Is there anything else I need to do with this?

[yuwata]

looks good to me now.

Please squash and force-push the commits. We usually merge PR in a perfect form, that is, without history of the patch.

[yuwata]

The CI Fedora Rawhide Compose x86_64 randomly fails. See other PRs. I do not think we should stick the failure.