Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Received-SPF: Pass (protection.outlook.com: domain of nvidia.com designates
 216.228.117.161 as permitted sender) receiver=protection.outlook.com;
 client-ip=216.228.117.161; helo=mail.nvidia.com; pr=C
References: <20221024091333.1048061-1-daniel.machon@microchip.com>
 <20221024091333.1048061-3-daniel.machon@microchip.com>
User-agent: mu4e 1.6.6; emacs 28.1
From:   Petr Machata <petrm@nvidia.com>
To:     Daniel Machon <daniel.machon@microchip.com>
CC:     <netdev@vger.kernel.org>, <davem@davemloft.net>,
        <petrm@nvidia.com>, <maxime.chevallier@bootlin.com>,
        <thomas.petazzoni@bootlin.com>, <edumazet@google.com>,
        <kuba@kernel.org>, <pabeni@redhat.com>,
        <lars.povlsen@microchip.com>, <Steen.Hegelund@microchip.com>,
        <UNGLinuxDriver@microchip.com>, <joe@perches.com>,
        <linux@armlinux.org.uk>, <horatiu.vultur@microchip.com>,
        <Julia.Lawall@inria.fr>, <vladimir.oltean@nxp.com>,
        <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>
Subject: Re: [net-next v3 2/6] net: dcb: add new apptrust attribute
Date:   Wed, 26 Oct 2022 13:06:13 +0200
In-Reply-To: <20221024091333.1048061-3-daniel.machon@microchip.com>
Message-ID: <87zgdizvfq.fsf@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 26 Oct 2022 11:17:03.2735
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: d6d0dd34-92a5-41d0-2b7e-08dab7439c00
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=43083d15-7273-40c1-b7db-39efd9ccc17a;Ip=[216.228.117.161];Helo=[mail.nvidia.com]
X-MS-Exchange-CrossTenant-AuthSource: CY4PEPF0000B8ED.namprd05.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6579
X-Spam-Status: No, score=-1.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Daniel Machon <daniel.machon@microchip.com> writes:

> Add new apptrust extension attributes to the 8021Qaz APP managed object.
>
> Two new attributes, DCB_ATTR_DCB_APP_TRUST_TABLE and
> DCB_ATTR_DCB_APP_TRUST, has been added. Trusted selectors are passed in
> the nested attribute DCB_ATTR_DCB_APP_TRUST, in order of precedence.
>
> The new attributes are meant to allow drivers, whose hw supports the
> notion of trust, to be able to set whether a particular app selector is
> trusted - and in which order.
>
> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> ---
>  include/net/dcbnl.h        |  4 ++
>  include/uapi/linux/dcbnl.h | 10 +++++
>  net/dcb/dcbnl.c            | 77 ++++++++++++++++++++++++++++++++++++--
>  3 files changed, 87 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/dcbnl.h b/include/net/dcbnl.h
> index 2b2d86fb3131..8841ab6c2de7 100644
> --- a/include/net/dcbnl.h
> +++ b/include/net/dcbnl.h
> @@ -109,6 +109,10 @@ struct dcbnl_rtnl_ops {
>  	/* buffer settings */
>  	int (*dcbnl_getbuffer)(struct net_device *, struct dcbnl_buffer *);
>  	int (*dcbnl_setbuffer)(struct net_device *, struct dcbnl_buffer *);
> +
> +	/* apptrust */
> +	int (*dcbnl_setapptrust)(struct net_device *, u8 *, int);
> +	int (*dcbnl_getapptrust)(struct net_device *, u8 *, int *);
>  };
>  
>  #endif /* __NET_DCBNL_H__ */
> diff --git a/include/uapi/linux/dcbnl.h b/include/uapi/linux/dcbnl.h
> index dc7ef96207ca..9344e3ba5768 100644
> --- a/include/uapi/linux/dcbnl.h
> +++ b/include/uapi/linux/dcbnl.h
> @@ -410,6 +410,7 @@ enum dcbnl_attrs {
>   * @DCB_ATTR_IEEE_PEER_ETS: peer ETS configuration - get only
>   * @DCB_ATTR_IEEE_PEER_PFC: peer PFC configuration - get only
>   * @DCB_ATTR_IEEE_PEER_APP: peer APP tlv - get only
> + * @DCB_ATTR_DCB_APP_TRUST_TABLE: selector trust table
>   */
>  enum ieee_attrs {
>  	DCB_ATTR_IEEE_UNSPEC,
> @@ -423,6 +424,7 @@ enum ieee_attrs {
>  	DCB_ATTR_IEEE_QCN,
>  	DCB_ATTR_IEEE_QCN_STATS,
>  	DCB_ATTR_DCB_BUFFER,
> +	DCB_ATTR_DCB_APP_TRUST_TABLE,
>  	__DCB_ATTR_IEEE_MAX
>  };
>  #define DCB_ATTR_IEEE_MAX (__DCB_ATTR_IEEE_MAX - 1)
> @@ -435,6 +437,14 @@ enum ieee_attrs_app {
>  };
>  #define DCB_ATTR_IEEE_APP_MAX (__DCB_ATTR_IEEE_APP_MAX - 1)
>  
> +enum dcbnl_attrs_apptrust {
> +	DCB_ATTR_DCB_APP_TRUST_UNSPEC,
> +	DCB_ATTR_DCB_APP_TRUST,
> +	__DCB_ATTR_DCB_APP_TRUST_MAX
> +};
> +
> +#define DCB_ATTR_DCB_APP_TRUST_MAX (__DCB_ATTR_DCB_APP_TRUST_MAX - 1)
> +
>  /**
>   * enum cee_attrs - CEE DCBX get attributes.
>   *
> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> index 92c32bc11374..01310edf6d1b 100644
> --- a/net/dcb/dcbnl.c
> +++ b/net/dcb/dcbnl.c
> @@ -166,6 +166,7 @@ static const struct nla_policy dcbnl_ieee_policy[DCB_ATTR_IEEE_MAX + 1] = {
>  	[DCB_ATTR_IEEE_QCN]         = {.len = sizeof(struct ieee_qcn)},
>  	[DCB_ATTR_IEEE_QCN_STATS]   = {.len = sizeof(struct ieee_qcn_stats)},
>  	[DCB_ATTR_DCB_BUFFER]       = {.len = sizeof(struct dcbnl_buffer)},
> +	[DCB_ATTR_DCB_APP_TRUST_TABLE] = {.type = NLA_NESTED},
>  };
>  
>  /* DCB number of traffic classes nested attributes. */
> @@ -1057,11 +1058,11 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb,
>  /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */
>  static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
>  {
> -	struct nlattr *ieee, *app;
> -	struct dcb_app_type *itr;
>  	const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops;
> +	struct nlattr *ieee, *app, *apptrust;
> +	struct dcb_app_type *itr;
> +	int err, i;
>  	int dcbx;
> -	int err;
>  
>  	if (nla_put_string(skb, DCB_ATTR_IFNAME, netdev->name))
>  		return -EMSGSIZE;
> @@ -1161,6 +1162,24 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
>  	spin_unlock_bh(&dcb_lock);
>  	nla_nest_end(skb, app);
>  
> +	if (ops->dcbnl_getapptrust) {
> +		u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0};
> +		int nselectors;
> +
> +		apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE);
> +		if (!app)
> +			return -EMSGSIZE;
> +
> +		err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors);
> +		if (err)
> +			return -EMSGSIZE;

This should return the error coming from the driver instead of
-EMSGSIZE.

Also, it should cancel the nest before returning.

> +
> +		for (i = 0; i < nselectors; i++)
> +			nla_put_u8(skb, DCB_ATTR_DCB_APP_TRUST, selectors[i]);
> +
> +		nla_nest_end(skb, apptrust);
> +	}
> +
>  	/* get peer info if available */
>  	if (ops->ieee_peer_getets) {
>  		struct ieee_ets ets;
> @@ -1454,8 +1473,8 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
>  {
>  	const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops;
>  	struct nlattr *ieee[DCB_ATTR_IEEE_MAX + 1];
> +	int err, i;
>  	int prio;
> -	int err;

I don't really mind, but I have to wonder why this new variable is not
with the rest of them in the trust table scope.

>  
>  	if (!ops)
>  		return -EOPNOTSUPP;
> @@ -1541,6 +1560,56 @@ static int dcbnl_ieee_set(struct net_device *netdev, struct nlmsghdr *nlh,
>  		}
>  	}
>  
> +	if (ieee[DCB_ATTR_DCB_APP_TRUST_TABLE]) {
> +		u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0};
> +		struct nlattr *attr;
> +		int nselectors = 0;
> +		u8 selector;
> +		int rem;
> +
> +		if (!ops->dcbnl_setapptrust) {
> +			err = -EOPNOTSUPP;
> +			goto err;
> +		}
> +
> +		nla_for_each_nested(attr, ieee[DCB_ATTR_DCB_APP_TRUST_TABLE],
> +				    rem) {
> +			if (nla_type(attr) != DCB_ATTR_DCB_APP_TRUST ||
> +			    nla_len(attr) != 1 ||
> +			    nselectors >= sizeof(selectors)) {
> +				err = -EINVAL;
> +				goto err;
> +			}
> +
> +			selector = nla_get_u8(attr);
> +			switch (selector) {
> +			case IEEE_8021QAZ_APP_SEL_ETHERTYPE:
> +			case IEEE_8021QAZ_APP_SEL_STREAM:
> +			case IEEE_8021QAZ_APP_SEL_DGRAM:
> +			case IEEE_8021QAZ_APP_SEL_ANY:
> +			case IEEE_8021QAZ_APP_SEL_DSCP:
> +			case DCB_APP_SEL_PCP:
> +				break;
> +			default:
> +				err = -EINVAL;
> +				goto err;
> +			}
> +			/* Duplicate selector ? */
> +			for (i = 0; i < nselectors; i++) {
> +				if (selectors[i] == selector) {
> +					err = -EINVAL;
> +					goto err;
> +				}
> +			}
> +
> +			selectors[nselectors++] = selector;
> +		}
> +
> +		err = ops->dcbnl_setapptrust(netdev, selectors, nselectors);
> +		if (err)
> +			goto err;
> +	}
> +
>  err:
>  	err = nla_put_u8(skb, DCB_ATTR_IEEE, err);
>  	dcbnl_ieee_notify(netdev, RTM_SETDCB, DCB_CMD_IEEE_SET, seq, 0);