Re: [RFC PATCH v2 3/5] tools/hotplug/Linux: Add bridge VLAN support

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, May 8, 2024 at 6:55 PM Leigh Brown <leigh@xxxxxxxxxxxxx> wrote:
>
> Update add_to_bridge shell function to read the vlan parameter
> from xenstore and set the bridge VLAN configuration for the VID.
>
> Add additional helper functions to parse the vlan specification,
> which consists of one or more of the follow:
>
> a) single VLAN (e.g. 10).
> b) contiguous range of VLANs (e.g. 10-15).
> c) discontiguous range with base, increment and count
>    (e.g. 100+10x9 which gives VLAN IDs 100, 110, ... 190).
>
> A single VLAN can be suffixed with "p" to indicate the PVID, or
> "u" to indicate untagged. A range of VLANs can be suffixed with
> "u" to indicate untagged.  A complex example would be:
>
>    vlan=1p/10-15/20-25u
>
> This capability only works when using the iproute2 bridge command,
> so a warning is issued if the vlan parameter is set and the bridge
> command is not available, as it will be ignored.
>
> Signed-off-by: Leigh Brown <leigh@xxxxxxxxxxxxx>
> ---
>  tools/hotplug/Linux/xen-network-common.sh | 111 ++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
>
> diff --git a/tools/hotplug/Linux/xen-network-common.sh 
> b/tools/hotplug/Linux/xen-network-common.sh
> index 42fa704e8d..d9fb4f7355 100644
> --- a/tools/hotplug/Linux/xen-network-common.sh
> +++ b/tools/hotplug/Linux/xen-network-common.sh
> @@ -121,10 +121,113 @@ create_bridge () {
>      fi
>  }
>
> +_vif_vlan_add() {
> +    # References vlans, pvid and error variables from the calling function
> +    local -i vid=$1
> +    local flag=${2:-}
> +
> +    if (( vid < 1 || vid > 4094 )) ;then
> +        error="vlan id $vid not between 1 and 4094"
> +        return
> +    fi
> +    if [[ -n "${vlans[$vid]}" ]] ;then
> +        error="vlan id $vid specified more than once"
> +        return

You could do `fatal "vlan id $vid specified more than once"` and just
terminate the script at this point.  It would simplify your later code
if you use fatal more.

> +    fi
> +    case $flag in
> +     p) if (( pvid != 0 )) ;then
> +            error="more than one pvid specified ($vid and $pvid)"
> +            return
> +        fi
> +        pvid=$vid
> +        vlans[$vid]=p ;;
> +     u) vlans[$vid]=u ;;
> +     *) vlans[$vid]=t ;;
> +    esac
> +}
> +
> +_vif_vlan_parse_term() {
> +    # References error variable from the calling function
> +    local vid incr last term=${1:-}
> +
> +    if [[ $term =~ ^([0-9]+)([pu])?$ ]] ;then

I like that you split the different cases into multiple REs.

> +        _vif_vlan_add ${BASH_REMATCH[1]} ${BASH_REMATCH[2]}
> +    elif [[ $term =~ ^([0-9]+)-([0-9]+)(u)?$ ]] ;then
> +        vid=${BASH_REMATCH[1]}
> +        last=${BASH_REMATCH[2]}
> +        if (( last >= vid )) ;then
> +            for (( ; vid<=last; vid++ )) ;do
> +                _vif_vlan_add $vid ${BASH_REMATCH[3]}
> +            done
> +        else
> +           error="invalid vlan id range: $term"
> +        fi
> +    elif [[ $term =~ ^([0-9]+)\+([0-9]+)x([0-9]+)(u)?$ ]] ;then
> +        vid=${BASH_REMATCH[1]}
> +        incr=${BASH_REMATCH[2]}
> +        for (( j=${BASH_REMATCH[3]}; j>0; --j, vid+=incr ))
> +        do
> +            _vif_vlan_add $vid ${BASH_REMATCH[4]}
> +        done
> +    else
> +        error="invalid vlan specification: $term"
> +    fi
> +}
> +
> +_vif_vlan_validate_pvid() {
> +    # References vlans and pvid variables from the calling function
> +    if (( pvid == 0 )) ;then
> +        if (( ${#vlans[@]} == 1 )) ;then
> +            vlans[${!vlans[*]}]=p
> +        else
> +            error="pvid required for multiple vlan ids"
> +        fi
> +    fi
> +}
> +
> +_vif_vlan_setup() {
> +    # References vlans and dev variable from the calling function
> +    local vid cmd
> +
> +    bridge vlan del dev "$dev" vid 1
> +    for vid in ${!vlans[@]} ;do
> +        cmd="bridge vlan add dev '$dev' vid $vid"
> +        case ${vlans[$vid]} in
> +             p) cmd="$cmd pvid untagged" ;;
> +             u) cmd="$cmd untagged" ;;
> +             t) ;;
> +        esac
> +        eval "$cmd"
> +    done
> +}
> +
> +_vif_vlan_membership() {
> +    # The vlans, pvid, dev and error variables are used by sub-functions
> +    local -A vlans=()
> +    local -a terms=()
> +    local -i i pvid=0
> +    local dev=$1 error=""
> +
> +    # Split the vlan specification string into its terms
> +    readarray -d / -t terms <<<$2
> +    for (( i=0; i<${#terms[@]}; ++i )) ;do
> +        _vif_vlan_parse_term ${terms[$i]%%[[:space:]]}
> +        [[ -n "$error" ]] && break
> +    done
> +
> +    [[ -z "$error" ]] && _vif_vlan_validate_pvid
> +    [[ -z "$error" ]] && _vif_vlan_setup
> +    [[ -z "$error" ]] && return 0
> +
> +    log error "$error"
> +    return 1
> +}
> +
>  # Usage: add_to_bridge bridge dev
>  add_to_bridge () {
>      local bridge=$1
>      local dev=$2
> +    local vlan=$(xenstore_read_default "$XENBUS_PATH/vlan" "")
>
>      # Don't add $dev to $bridge if it's already on the bridge.
>      if [ ! -e "/sys/class/net/${bridge}/brif/${dev}" ]; then
> @@ -134,6 +237,14 @@ add_to_bridge () {
>          else
>              ip link set ${dev} master ${bridge}
>          fi
> +        if [ -n "${vlan}" ] ;then
> +            if which bridge >&/dev/null; then
> +                log debug "configuring VLANs for ${dev} on ${bridge}"
> +                _vif_vlan_membership "${dev}" "${vlan}"
> +            else
> +                log warning "bridge command not available, ignoring vlan 
> config"

Do you think this should be fatal?  It seems to me that setting up the
connection but not applying the vlans could be a security issue.

This file before your patch was very close to posix sh.  Afterwards,
it definitely needs bash.  vif-bridge is /bin/bash, so it is fine.

Regards,
Jason