On Thu, Aug 21, 2025 at 11:24 AM David Marchand <david.march...@redhat.com>
wrote:
> Protect concurrent access when calling netdev_get_features()
> or netdev_get_speed() for BSD netdevs.
>
> Signed-off-by: David Marchand <david.march...@redhat.com>
> ---
> lib/netdev-bsd.c | 111 +++++++++++++++++++++++++++++------------------
> 1 file changed, 68 insertions(+), 43 deletions(-)
>
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index c7cd59376a..7718c30a04 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -92,6 +92,11 @@ struct netdev_bsd {
> struct eth_addr etheraddr;
> int mtu;
> int carrier;
> + int get_features_error;
> +
> + enum netdev_features current;
> + enum netdev_features advertised;
> + enum netdev_features supported;
>
> int tap_fd; /* TAP character device, if any, otherwise -1. */
>
> @@ -1099,28 +1104,22 @@ netdev_bsd_parse_media(int media)
> return supported;
> }
>
> -/*
> - * Stores the features supported by 'netdev' into each of '*current',
> - * '*advertised', '*supported', and '*peer' that are non-null. Each
> value is a
> - * bitmap of "enum ofp_port_features" bits, in host byte order. Returns
> 0 if
> - * successful, otherwise a positive errno value. On failure, all of the
> - * passed-in values are set to 0.
> - */
> -static int
> -netdev_bsd_get_features(const struct netdev *netdev,
> - enum netdev_features *current, uint32_t
> *advertised,
> - enum netdev_features *supported, uint32_t *peer)
> +static void
> +netdev_bsd_read_features(struct netdev_bsd *netdev)
> {
> struct ifmediareq ifmr;
> int *media_list;
> - int i;
> int error;
> -
> + int i;
>
> /* XXX Look into SIOCGIFCAP instead of SIOCGIFMEDIA */
>
> memset(&ifmr, 0, sizeof(ifmr));
> - ovs_strlcpy(ifmr.ifm_name, netdev_get_name(netdev), sizeof
> ifmr.ifm_name);
> + ovs_strlcpy(ifmr.ifm_name, netdev_get_name(&netdev->up),
> + sizeof ifmr.ifm_name);
> +
> + media_list = xcalloc(ifmr.ifm_count, sizeof(int));
> + ifmr.ifm_ulist = media_list;
>
> /* We make two SIOCGIFMEDIA ioctl calls. The first to determine the
> * number of supported modes, and a second with a buffer to retrieve
> @@ -1128,16 +1127,13 @@ netdev_bsd_get_features(const struct netdev
> *netdev,
> error = af_inet_ioctl(SIOCGIFMEDIA, &ifmr);
> if (error) {
> VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
> - netdev_get_name(netdev), ovs_strerror(error));
> - return error;
> + netdev_get_name(&netdev->up), ovs_strerror(error));
> + goto cleanup;
> }
>
> - media_list = xcalloc(ifmr.ifm_count, sizeof(int));
> - ifmr.ifm_ulist = media_list;
> -
> if (IFM_TYPE(ifmr.ifm_current) != IFM_ETHER) {
> VLOG_DBG_RL(&rl, "%s: doesn't appear to be ethernet",
> - netdev_get_name(netdev));
> + netdev_get_name(&netdev->up));
> error = EINVAL;
> goto cleanup;
> }
> @@ -1145,53 +1141,82 @@ netdev_bsd_get_features(const struct netdev
> *netdev,
> error = af_inet_ioctl(SIOCGIFMEDIA, &ifmr);
> if (error) {
> VLOG_DBG_RL(&rl, "%s: ioctl(SIOCGIFMEDIA) failed: %s",
> - netdev_get_name(netdev), ovs_strerror(error));
> + netdev_get_name(&netdev->up), ovs_strerror(error));
> goto cleanup;
> }
>
> /* Current settings. */
> - *current = netdev_bsd_parse_media(ifmr.ifm_active);
> - if (!*current) {
> - *current = NETDEV_F_OTHER;
> + netdev->current = netdev_bsd_parse_media(ifmr.ifm_active);
> + if (!netdev->current) {
> + netdev->current = NETDEV_F_OTHER;
> }
>
> /* Advertised features. */
> - *advertised = netdev_bsd_parse_media(ifmr.ifm_current);
> + netdev->advertised = netdev_bsd_parse_media(ifmr.ifm_current);
>
> /* Supported features. */
> - *supported = 0;
> + netdev->supported = 0;
> for (i = 0; i < ifmr.ifm_count; i++) {
> - *supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]);
> + netdev->supported |= netdev_bsd_parse_media(ifmr.ifm_ulist[i]);
> }
>
> - /* Peer advertisements. */
> - *peer = 0; /* XXX */
> -
> error = 0;
> +
> cleanup:
> free(media_list);
> + netdev->get_features_error = error;
>
Why not just return this value?
> +}
> +
> +/*
> + * Stores the features supported by 'netdev' into each of '*current',
> + * '*advertised', '*supported', and '*peer' that are non-null. Each
> value is a
> + * bitmap of "enum ofp_port_features" bits, in host byte order. Returns
> 0 if
> + * successful, otherwise a positive errno value. On failure, all of the
> + * passed-in values are set to 0.
> + */
> +static int
> +netdev_bsd_get_features(const struct netdev *netdev_,
> + enum netdev_features *current, uint32_t
> *advertised,
> + enum netdev_features *supported, uint32_t *peer)
> +{
> + struct netdev_bsd *netdev = netdev_bsd_cast(netdev_);
> + int error;
> +
> + ovs_mutex_lock(&netdev->mutex);
> + netdev_bsd_read_features(netdev);
> + if (!netdev->get_features_error) {
> + *current = netdev->current;
> + *advertised = netdev->advertised;
> + *supported = netdev->supported;
> + *peer = 0;
> + }
> + error = netdev->get_features_error;
> + ovs_mutex_unlock(&netdev->mutex);
> +
> return error;
> }
>
> static int
> -netdev_bsd_get_speed(const struct netdev *netdev, uint32_t *current,
> +netdev_bsd_get_speed(const struct netdev *netdev_, uint32_t *current,
> uint32_t *max)
> {
> - enum netdev_features f_current, f_supported, f_advertised, f_peer;
> + struct netdev_bsd *netdev = netdev_bsd_cast(netdev_);
> int error;
>
> - error = netdev_bsd_get_features(netdev, &f_current, &f_advertised,
> - &f_supported, &f_peer);
> - if (error) {
> - return error;
> - }
> -
> - *current = MIN(UINT32_MAX,
> - netdev_features_to_bps(f_current, 0) / 1000000ULL);
> - *max = MIN(UINT32_MAX,
> - netdev_features_to_bps(f_supported, 0) / 1000000ULL);
> + ovs_mutex_lock(&netdev->mutex);
> + netdev_bsd_read_features(netdev);
> + if (!netdev->get_features_error) {
> + *current = MIN(UINT32_MAX,
> + netdev_features_to_bps(netdev->current, 0)
> + / 1000000ULL);
> + *max = MIN(UINT32_MAX,
> + netdev_features_to_bps(netdev->supported, 0)
> + / 1000000ULL);
> + }
> + error = netdev->get_features_error;
> + ovs_mutex_unlock(&netdev->mutex);
>
> - return 0;
> + return error;
> }
>
> /*
> --
> 2.50.1
>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
