Received-SPF: pass (google.com: best guess record for domain of linux-wireless-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200313065114.23433-1-yhchuang@realtek.com>
In-Reply-To: <20200313065114.23433-1-yhchuang@realtek.com>
From:   Brian Norris <briannorris@chromium.org>
Date:   Thu, 26 Mar 2020 11:27:54 -0700
Message-ID: <CA+ASDXNZxBZxVxzdNLiOZKwoQyWB5Pph=AeMW5gJ4cmMKigKcg@mail.gmail.com>
Subject: Re: [PATCH] rtw88: add debugfs to fix tx rate
To:     Tony Chuang <yhchuang@realtek.com>
Cc:     Kalle Valo <kvalo@codeaurora.org>,
        linux-wireless <linux-wireless@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

I know there's other discussion on this patch, related to nl80211 vs.
debugfs, but in case the discussion ends up permitting debugfs, I'll
put my review notes in.

Reviewed-by: Brian Norris <briannorris@chromium.org>
Tested-by: Brian Norris <briannorris@chromium.org>

On Thu, Mar 12, 2020 at 11:51 PM <yhchuang@realtek.com> wrote:
> +static ssize_t rtw_debugfs_set_fix_rate(struct file *filp,
> +                                       const char __user *buffer,
> +                                       size_t count, loff_t *loff)
> +{
> +       struct seq_file *seqpriv = (struct seq_file *)filp->private_data;
> +       struct rtw_debugfs_priv *debugfs_priv = seqpriv->private;
> +       struct rtw_dev *rtwdev = debugfs_priv->rtwdev;
> +       struct rtw_dm_info *dm_info = &rtwdev->dm_info;
> +       u8 fix_rate;
> +       char tmp[32 + 1];
> +       int ret;
> +
> +       rtw_debugfs_copy_from_user(tmp, sizeof(tmp), buffer, count, 1);
> +
> +       ret = kstrtou8(tmp, 0, &fix_rate);
> +       if (ret) {
> +               rtw_warn(rtwdev, "invalid args, [rate]\n");

Seems like you don't actually need this print, as it doesn't provide
any additional data that you don't get through the return code. People
interacting with this debugfs interface will already see things like
"write error: Invalid argument" if they're handling write() errors
appropriately.

> +               return ret;
> +       }
> +
> +       dm_info->fix_rate = fix_rate;

It feels like you should do some real bounds checking here; as-is,
you're allowing anything in [DESC_RATE_MAX..0xff] to mean "disabled",
whereas it seems much more reasonable to specify a single value that
means "disabled". So you could do:

  if (fix_rate >= DESC_RATE_MAX && fix_rate != U8_MAX)
    return -EINVAL;

Brian

> +
> +       return count;
> +}
> +