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;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.120.0.1.13\))
Subject: Re: [PATCH 1/4] Bluetooth: Add bt_skb_sendmsg helper
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <20210901002621.414016-1-luiz.dentz@gmail.com>
Date:   Wed, 1 Sep 2021 07:44:48 +0200
Cc:     linux-bluetooth@vger.kernel.org
Content-Transfer-Encoding: 8BIT
Message-Id: <9067109F-D8FD-47E9-AD02-FB6B7DFFDB3E@holtmann.org>
References: <20210901002621.414016-1-luiz.dentz@gmail.com>
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Precedence: bulk

Hi Luiz,

> bt_skb_sendmsg helps takes care of allocation the skb and copying the
> the contents of msg over to the skb while checking for possible errors
> so it should be safe to call it without holding lock_sock.
> 
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> include/net/bluetooth/bluetooth.h | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
> 
> diff --git a/include/net/bluetooth/bluetooth.h b/include/net/bluetooth/bluetooth.h
> index 9125effbf448..f858efcf9f40 100644
> --- a/include/net/bluetooth/bluetooth.h
> +++ b/include/net/bluetooth/bluetooth.h
> @@ -420,6 +420,32 @@ static inline struct sk_buff *bt_skb_send_alloc(struct sock *sk,
> 	return NULL;
> }
> 
> +/* Shall not be called with lock_sock held */
> +static inline struct sk_buff *bt_skb_sendmsg(struct sock *sk,
> +					     struct msghdr *msg,
> +					     size_t len, size_t header,
> +					     size_t footer)
> +{
> +	struct sk_buff *skb;
> +	int err;
> +
> +	skb = bt_skb_send_alloc(sk, len + header + footer,
> +				msg->msg_flags & MSG_DONTWAIT, &err);
> +	if (!skb)
> +		return ERR_PTR(err);
> +
> +	skb_reserve(skb, header);

I am not with you on this one since bt_skb_send_alloc already calls skb_reserve in the first place.

/**                                                                              
 *      skb_reserve - adjust headroom                                            
 *      @skb: buffer to alter                                                    
 *      @len: bytes to move                                                      
 *                                                                               
 *      Increase the headroom of an empty &sk_buff by reducing the tail          
 *      room. This is only allowed for an empty buffer.                          
 */

In addition we have this comment here. So what kind of headroom do we need with this SKB?

And wouldn’t it be better to actually assign a tailroom instead of just a large enough buffer?

/**                                                                              
 *      skb_tailroom_reserve - adjust reserved_tailroom                          
 *      @skb: buffer to alter                                                    
 *      @mtu: maximum amount of headlen permitted                                
 *      @needed_tailroom: minimum amount of reserved_tailroom                    
 *                                                                               
 *      Set reserved_tailroom so that headlen can be as large as possible but    
 *      not larger than mtu and tailroom cannot be smaller than                  
 *      needed_tailroom.                                                         
 *      The required headroom should already have been reserved before using     
 *      this function.                                                           
 */              

We also have this capability inside the SKBs.

So while the basic idea of this patchset seems fine, we need to figure out the details and not overload us in spaghetti code by wanting to have a common bt_skb_* helper. Maybe it is actually not helpful in this case since we just have to have a too large parameter list to satisfy all 3 users.

Regards

Marcel