| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <IA3PR11MB8986ECE1CEEFDE1E6972BE75E5F3A@IA3PR11MB8986.namprd11.prod.outlook.com>
Date: Wed, 22 Oct 2025 09:58:06 +0000
From: "Loktionov, Aleksandr" <aleksandr.loktionov@...el.com>
To: Jakub Kicinski <kuba@...nel.org>, "Keller, Jacob E"
<jacob.e.keller@...el.com>
CC: Jiri Pirko <jiri@...nulli.us>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, "Simon
Horman" <horms@...nel.org>, Jonathan Corbet <corbet@....net>, "Nguyen,
Anthony L" <anthony.l.nguyen@...el.com>, "Kitszel, Przemyslaw"
<przemyslaw.kitszel@...el.com>, Andrew Lunn <andrew+netdev@...n.ch>,
"Lobakin, Aleksander" <aleksander.lobakin@...el.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Nowlin, Dan"
<dan.nowlin@...el.com>, "Wang, Jie1X" <jie1x.wang@...el.com>, Junfeng Guo
<junfeng.guo@...el.com>, "Zhang, Qi Z" <qi.z.zhang@...el.com>, Ting Xu
<ting.xu@...el.com>, "Romanowski, Rafal" <rafal.romanowski@...el.com>
Subject: RE: [PATCH net-next v2 04/14] ice: add virtchnl and VF context
support for GTP RSS
> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: Tuesday, October 21, 2025 3:23 AM
> To: Keller, Jacob E <jacob.e.keller@...el.com>
> Cc: Jiri Pirko <jiri@...nulli.us>; David S. Miller
> <davem@...emloft.net>; Eric Dumazet <edumazet@...gle.com>; Paolo Abeni
> <pabeni@...hat.com>; Simon Horman <horms@...nel.org>; Jonathan Corbet
> <corbet@....net>; Nguyen, Anthony L <anthony.l.nguyen@...el.com>;
> Kitszel, Przemyslaw <przemyslaw.kitszel@...el.com>; Andrew Lunn
> <andrew+netdev@...n.ch>; Lobakin, Aleksander
> <aleksander.lobakin@...el.com>; netdev@...r.kernel.org; linux-
> doc@...r.kernel.org; linux-kernel@...r.kernel.org; Loktionov,
> Aleksandr <aleksandr.loktionov@...el.com>; Nowlin, Dan
> <dan.nowlin@...el.com>; Wang, Jie1X <jie1x.wang@...el.com>; Junfeng
> Guo <junfeng.guo@...el.com>; Zhang, Qi Z <qi.z.zhang@...el.com>; Ting
> Xu <ting.xu@...el.com>; Romanowski, Rafal <rafal.romanowski@...el.com>
> Subject: Re: [PATCH net-next v2 04/14] ice: add virtchnl and VF
> context support for GTP RSS
>
> On Thu, 16 Oct 2025 23:08:33 -0700 Jacob Keller wrote:
> > void ice_parser_destroy(struct ice_parser *psr) {
> > + if (!psr)
> > + return;
>
> questionable
>
But it potentially simplifies cleanup code in different functions (no need for additional if (psr) in functions).
Do you insist to move it from here?
> > + {VIRTCHNL_PROTO_HDR_L2TPV2,
> > + FIELD_SELECTOR(VIRTCHNL_PROTO_HDR_L2TPV2_SESS_ID),
> > + BIT_ULL(ICE_FLOW_FIELD_IDX_L2TPV2_SESS_ID)},
> > + {VIRTCHNL_PROTO_HDR_L2TPV2,
> > + FIELD_SELECTOR(VIRTCHNL_PROTO_HDR_L2TPV2_LEN_SESS_ID),
> > + BIT_ULL(ICE_FLOW_FIELD_IDX_L2TPV2_LEN_SESS_ID)},
>
> consider moving out all the static data and define changes to a
> separate commit for ease of review?
>
Ok, will do my best. Patch #2 is big, but everything is related to the context.
> > };
> >
> > +static enum virtchnl_status_code
> > +ice_vc_rss_hash_update(struct ice_hw *hw, struct ice_vsi *vsi, u8
> > +hash_type) {
> > + enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
> > + struct ice_vsi_ctx *ctx;
> > + int ret;
> > +
> > + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
> > + if (!ctx)
> > + return VIRTCHNL_STATUS_ERR_NO_MEMORY;
>
> I feel like the VIRTCHNL_* error codes are spreading, we've been over
> this.
>
I can understand your feelings.
But it's:
- static function (only used within this file)
- only called from virtchnl handler functions
- "vc" (virtchnl) in its name indicating it's part of the virtchnl layer
- not exposed in any headers
I think this is appropriate because it's part of the virtchnl abstraction layer and only used within virtchnl handlers.
> > +static int
> > +ice_hash_remove(struct ice_vf *vf, struct ice_rss_hash_cfg *cfg) {
> > + int ret;
> > +
> > + ret = ice_hash_moveout(vf, cfg);
> > + if (ret && (ret != -ENOENT))
>
> spurious brackets
>
Yep, have to fix.
> > +/**
> > + * ice_add_rss_cfg_pre_ip - Pre-process IP-layer RSS configuration
> > + * @vf: VF pointer
> > + * @ctx: IP L4 hash context (ESP/UDP-ESP/AH/PFCP and UDP/TCP/SCTP)
> > + *
> > + * Remove covered/recorded IP RSS configurations prior to adding a
> new one.
> > + *
> > + * Return: 0 on success; negative error code on failure.
> > + */
> > +static int
> > +ice_add_rss_cfg_pre_ip(struct ice_vf *vf, struct ice_vf_hash_ip_ctx
> > +*ctx) {
> > + int i, ret;
> > +
> > + for (i = 1; i < ICE_HASH_IP_CTX_MAX; i++)
> > + if (ice_is_hash_cfg_valid(&ctx->ctx[i])) {
> > + ret = ice_hash_remove(vf, &ctx->ctx[i]);
> > +
>
> spurious new line
>
Will fix it.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> > +static int
> > +ice_parse_raw_rss_pattern(struct ice_vf *vf, struct
> virtchnl_proto_hdrs *proto,
> > + struct ice_rss_raw_cfg *raw_cfg) {
> > + struct ice_parser_result pkt_parsed;
> > + struct ice_hw *hw = &vf->pf->hw;
> > + struct ice_parser_profile prof;
> > + u16 pkt_len;
> > + struct ice_parser *psr;
> > + u8 *pkt_buf, *msk_buf;
> > + int ret = 0;
> > +
> > + pkt_len = proto->raw.pkt_len;
> > + if (!pkt_len)
> > + return -EINVAL;
> > + if (pkt_len > VIRTCHNL_MAX_SIZE_RAW_PACKET)
> > + pkt_len = VIRTCHNL_MAX_SIZE_RAW_PACKET;
> > +
> > + pkt_buf = kzalloc(pkt_len, GFP_KERNEL);
> > + msk_buf = kzalloc(pkt_len, GFP_KERNEL);
> > + if (!pkt_buf || !msk_buf) {
> > + ret = -ENOMEM;
> > + goto free_alloc;
> > + }
> > +
> > + memcpy(pkt_buf, proto->raw.spec, pkt_len);
> > + memcpy(msk_buf, proto->raw.mask, pkt_len);
> > +
> > + psr = ice_parser_create(hw);
> > + if (IS_ERR(psr)) {
> > + ret = PTR_ERR(psr);
> > + goto parser_destroy;
>
> goto free_alloc, surely, parser creation has failed, there's nothing
> to destroy
>
Agree, easy fix.
> > + }
> > +
> > + ret = ice_parser_run(psr, pkt_buf, pkt_len, &pkt_parsed);
> > + if (ret)
> > + goto parser_destroy;
> > +
> > + ret = ice_parser_profile_init(&pkt_parsed, pkt_buf, msk_buf,
> > + pkt_len, ICE_BLK_RSS, &prof);
> > + if (ret)
> > + goto parser_destroy;
> > +
> > + memcpy(&raw_cfg->prof, &prof, sizeof(prof));
> > +
> > +parser_destroy:
> > + ice_parser_destroy(psr);
> > +free_alloc:
> > + kfree(pkt_buf);
> > + kfree(msk_buf);
> > + return ret;
> > +}
Thank you for reviewing
Alex
Powered by blists - more mailing lists