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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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