| 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: <CAL9ddJff2CKBfzSP8Mx-9PiS6APPMRHB72YAiwER7f4TLNFR+w@mail.gmail.com>
Date: Wed, 2 Sep 2020 13:39:18 -0700
From: David Awogbemila <awogbemila@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Kuo Zhao <kuozhao@...gle.com>,
Yangchun Fu <yangchun@...gle.com>
Subject: Re: [PATCH net-next v2 5/9] gve: Add Gvnic stats AQ command and
ethtool show/set-priv-flags.
On Tue, Sep 1, 2020 at 5:46 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Tue, 1 Sep 2020 14:51:45 -0700 David Awogbemila wrote:
>
> > @@ -297,6 +317,22 @@ static inline void gve_clear_probe_in_progress(struct gve_priv *priv)
> > clear_bit(GVE_PRIV_FLAGS_PROBE_IN_PROGRESS, &priv->service_task_flags);
> > }
> >
> > +static inline bool gve_get_do_report_stats(struct gve_priv *priv)
> > +{
> > + return test_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS,
> > + &priv->service_task_flags);
> > +}
> > +
> > +static inline void gve_set_do_report_stats(struct gve_priv *priv)
> > +{
> > + set_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> > +}
> > +
> > +static inline void gve_clear_do_report_stats(struct gve_priv *priv)
> > +{
> > + clear_bit(GVE_PRIV_FLAGS_DO_REPORT_STATS, &priv->service_task_flags);
> > +}
> > +
> > static inline bool gve_get_admin_queue_ok(struct gve_priv *priv)
> > {
> > return test_bit(GVE_PRIV_FLAGS_ADMIN_QUEUE_OK, &priv->state_flags);
> > @@ -357,6 +393,21 @@ static inline void gve_clear_napi_enabled(struct gve_priv *priv)
> > clear_bit(GVE_PRIV_FLAGS_NAPI_ENABLED, &priv->state_flags);
> > }
> >
> > +static inline bool gve_get_report_stats(struct gve_priv *priv)
> > +{
> > + return test_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
> > +
> > +static inline void gve_set_report_stats(struct gve_priv *priv)
>
> Please remove the unused helpers.
Thanks, I'll fix this.
>
> > +{
> > + set_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
> > +
> > +static inline void gve_clear_report_stats(struct gve_priv *priv)
> > +{
> > + clear_bit(GVE_PRIV_FLAGS_REPORT_STATS, &priv->ethtool_flags);
> > +}
>
> > @@ -353,6 +377,54 @@ static int gve_set_tunable(struct net_device *netdev,
> > }
> > }
> >
> > +static u32 gve_get_priv_flags(struct net_device *netdev)
> > +{
> > + struct gve_priv *priv = netdev_priv(netdev);
> > + u32 i, ret_flags = 0;
> > +
> > + for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {
>
> Please remove this pointless loop.
Thanks, I'll fix this.
>
> > + if (priv->ethtool_flags & BIT(i))
> > + ret_flags |= BIT(i);
> > + }
> > + return ret_flags;
> > +}
> > +
> > +static int gve_set_priv_flags(struct net_device *netdev, u32 flags)
> > +{
> > + struct gve_priv *priv = netdev_priv(netdev);
> > + u64 ori_flags, new_flags;
> > + u32 i;
> > +
> > + ori_flags = READ_ONCE(priv->ethtool_flags);
> > + new_flags = ori_flags;
> > +
> > + for (i = 0; i < GVE_PRIV_FLAGS_STR_LEN; i++) {
>
> Ditto.
Thanks, I'll fix this.
>
> > + if (flags & BIT(i))
> > + new_flags |= BIT(i);
> > + else
> > + new_flags &= ~(BIT(i));
> > + priv->ethtool_flags = new_flags;
> > + /* set report-stats */
> > + if (strcmp(gve_gstrings_priv_flags[i], "report-stats") == 0) {
> > + /* update the stats when user turns report-stats on */
> > + if (flags & BIT(i))
> > + gve_handle_report_stats(priv);
> > + /* zero off gve stats when report-stats turned off */
> > + if (!(flags & BIT(i)) && (ori_flags & BIT(i))) {
> > + int tx_stats_num = GVE_TX_STATS_REPORT_NUM *
> > + priv->tx_cfg.num_queues;
> > + int rx_stats_num = GVE_RX_STATS_REPORT_NUM *
> > + priv->rx_cfg.num_queues;
>
> new line here
Thanks, I'll fix this.
>
> > + memset(priv->stats_report->stats, 0,
> > + (tx_stats_num + rx_stats_num) *
> > + sizeof(struct stats));
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
>
>
> > +static int gve_alloc_stats_report(struct gve_priv *priv)
> > +{
> > + int tx_stats_num, rx_stats_num;
> > +
> > + tx_stats_num = (GVE_TX_STATS_REPORT_NUM) *
> > + priv->tx_cfg.num_queues;
> > + rx_stats_num = (GVE_RX_STATS_REPORT_NUM) *
> > + priv->rx_cfg.num_queues;
> > + priv->stats_report_len = sizeof(struct gve_stats_report) +
> > + (tx_stats_num + rx_stats_num) *
> > + sizeof(struct stats);
> > + priv->stats_report =
> > + dma_alloc_coherent(&priv->pdev->dev, priv->stats_report_len,
> > + &priv->stats_report_bus, GFP_KERNEL);
> > + if (!priv->stats_report)
> > + return -ENOMEM;
> > + /* Set up timer for periodic task */
> > + timer_setup(&priv->service_timer, gve_service_timer, 0);
> > + priv->service_timer_period = GVE_SERVICE_TIMER_PERIOD;
> > + /* Start the service task timer */
> > + mod_timer(&priv->service_timer,
> > + round_jiffies(jiffies +
> > + msecs_to_jiffies(priv->service_timer_period)));
> > + return 0;
> > +}
>
> > @@ -1173,6 +1315,7 @@ static int gve_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > priv->db_bar2 = db_bar;
> > priv->service_task_flags = 0x0;
> > priv->state_flags = 0x0;
> > + priv->ethtool_flags = 0x0;
> > priv->dma_mask = dma_mask;
>
> You allocate the memory and start the timer even tho the priv flag
> defaults to off?
That's correct. But the allocated space is still written to by the NIC
and those stats would still be available to the guest/driver.
Powered by blists - more mailing lists