Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210722074208.8040-1-penguin-kernel@I-love.SAKURA.ne.jp>
In-Reply-To: <20210722074208.8040-1-penguin-kernel@I-love.SAKURA.ne.jp>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Fri, 23 Jul 2021 14:44:05 -0700
Message-ID: <CABBYNZKmF7vODFxkDyRwFsTd933mNNB3vwVOCcxsgof_St=ORw@mail.gmail.com>
Subject: Re: [PATCH] Bluetooth: reorganize functions from hci_sock_sendmsg()
To:     Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc:     Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        LinMa <linma@zju.edu.cn>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Tetsuo,

On Thu, Jul 22, 2021 at 12:42 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Since userfaultfd mechanism allows sleeping with kernel lock held,
> avoiding page fault with kernel lock held where possible will make
> the module more robust. This patch just brings memcpy_from_msg() calls
> to out of sock lock.
>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Reviewed-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

> ---
>  net/bluetooth/hci_sock.c | 50 +++++++++++++++++-----------------------
>  1 file changed, 21 insertions(+), 29 deletions(-)
>
> diff --git a/net/bluetooth/hci_sock.c b/net/bluetooth/hci_sock.c
> index ef7fc3e9d471..7fac44fb771f 100644
> --- a/net/bluetooth/hci_sock.c
> +++ b/net/bluetooth/hci_sock.c
> @@ -1535,10 +1535,8 @@ static int hci_sock_recvmsg(struct socket *sock, struct msghdr *msg,
>         return err ? : copied;
>  }
>
> -static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
> -                       struct msghdr *msg, size_t msglen)
> +static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk, void *buf, size_t msglen)
>  {
> -       void *buf;
>         u8 *cp;
>         struct mgmt_hdr *hdr;
>         u16 opcode, index, len;
> @@ -1552,15 +1550,6 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
>         if (msglen < sizeof(*hdr))
>                 return -EINVAL;
>
> -       buf = kmalloc(msglen, GFP_KERNEL);
> -       if (!buf)
> -               return -ENOMEM;
> -
> -       if (memcpy_from_msg(buf, msg, msglen)) {
> -               err = -EFAULT;
> -               goto done;
> -       }
> -
>         hdr = buf;
>         opcode = __le16_to_cpu(hdr->opcode);
>         index = __le16_to_cpu(hdr->index);
> @@ -1657,11 +1646,10 @@ static int hci_mgmt_cmd(struct hci_mgmt_chan *chan, struct sock *sk,
>         if (hdev)
>                 hci_dev_put(hdev);
>
> -       kfree(buf);
>         return err;
>  }
>
> -static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
> +static int hci_logging_frame(struct sock *sk, void *buf, int len, unsigned int flags)
>  {
>         struct hci_mon_hdr *hdr;
>         struct sk_buff *skb;
> @@ -1676,14 +1664,11 @@ static int hci_logging_frame(struct sock *sk, struct msghdr *msg, int len)
>         if (len < sizeof(*hdr) + 3)
>                 return -EINVAL;
>
> -       skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
> +       skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
>         if (!skb)
>                 return err;
>
> -       if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> -               err = -EFAULT;
> -               goto drop;
> -       }
> +       memcpy(skb_put(skb, len), buf, len);
>
>         hdr = (void *)skb->data;
>
> @@ -1753,19 +1738,28 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>         struct hci_dev *hdev;
>         struct sk_buff *skb;
>         int err;
> +       void *buf;
> +       const unsigned int flags = msg->msg_flags;
>
>         BT_DBG("sock %p sk %p", sock, sk);
>
> -       if (msg->msg_flags & MSG_OOB)
> +       if (flags & MSG_OOB)
>                 return -EOPNOTSUPP;
>
> -       if (msg->msg_flags & ~(MSG_DONTWAIT|MSG_NOSIGNAL|MSG_ERRQUEUE|
> -                              MSG_CMSG_COMPAT))
> +       if (flags & ~(MSG_DONTWAIT | MSG_NOSIGNAL | MSG_ERRQUEUE | MSG_CMSG_COMPAT))
>                 return -EINVAL;
>
>         if (len < 4 || len > HCI_MAX_FRAME_SIZE)
>                 return -EINVAL;
>
> +       buf = kmalloc(len, GFP_KERNEL);
> +       if (!buf)
> +               return -ENOMEM;
> +       if (memcpy_from_msg(buf, msg, len)) {
> +               kfree(buf);
> +               return -EFAULT;
> +       }
> +
>         lock_sock(sk);
>
>         switch (hci_pi(sk)->channel) {
> @@ -1776,13 +1770,13 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>                 err = -EOPNOTSUPP;
>                 goto done;
>         case HCI_CHANNEL_LOGGING:
> -               err = hci_logging_frame(sk, msg, len);
> +               err = hci_logging_frame(sk, buf, len, flags);
>                 goto done;
>         default:
>                 mutex_lock(&mgmt_chan_list_lock);
>                 chan = __hci_mgmt_chan_find(hci_pi(sk)->channel);
>                 if (chan)
> -                       err = hci_mgmt_cmd(chan, sk, msg, len);
> +                       err = hci_mgmt_cmd(chan, sk, buf, len);
>                 else
>                         err = -EINVAL;
>
> @@ -1801,14 +1795,11 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>                 goto done;
>         }
>
> -       skb = bt_skb_send_alloc(sk, len, msg->msg_flags & MSG_DONTWAIT, &err);
> +       skb = bt_skb_send_alloc(sk, len, flags & MSG_DONTWAIT, &err);
>         if (!skb)
>                 goto done;
>
> -       if (memcpy_from_msg(skb_put(skb, len), msg, len)) {
> -               err = -EFAULT;
> -               goto drop;
> -       }
> +       memcpy(skb_put(skb, len), buf, len);
>
>         hci_skb_pkt_type(skb) = skb->data[0];
>         skb_pull(skb, 1);
> @@ -1880,6 +1871,7 @@ static int hci_sock_sendmsg(struct socket *sock, struct msghdr *msg,
>
>  done:
>         release_sock(sk);
> +       kfree(buf);
>         return err;
>
>  drop:
> --
> 2.18.4
>


-- 
Luiz Augusto von Dentz