[PATCH v4 0/4] Handling extended headers of bnep control frames

[PATCH v4 0/4] Handling extended headers of bnep control frames

[Grzegorz Kolodziejczyk]

This patch set adds support for handling extended headers of bnep control
frames which is mandatory. Until now extended headers for control messages was
ignored.

Patch [1/4] adds setting of err variable if control message is not understood.
Patch [2/4] adds additional ioctl for get supported bnep features. This is
needed for user space to know if handling of sending success setup response
frame should be handled by kernel or by user space.
Patch [3/4] adds support for handling extended headers of bnep control frames.
Patch [4/4] adds support for handling connection setup request.

Patch set tested with PTS bnep test cases. No regression, issues found during
testing.

v2.
- Added ioctl definition to compat_ioctl,
- Distinct bnep session flags and bnep features.

v3.
- Splited extended header, connection setup request handling into separated
	patches,
- Fixed flags, supported features defines as Marcel pointed,
- Extended commit messages,
- Remove copying flags from us to kernel bnep session,
- Fix setting bnep session bits as Marcel pointed.

v4.
- Unify internal/us flags usage like in HIDP, CMTP,
- Refactor send setup response as Marcel suggested,
- Make supported feature variable as local ioctl variable and set it while
adding session if required.

Grzegorz Kolodziejczyk (4):
  Bluetooth: bnep: Return err value while sending cmd is not understood
  Bluetooth: bnep: Add support for get bnep features via ioctl
  Bluetooth: bnep: Add support to extended headers of control frames
  Bluetooth: bnep: Handle BNEP connection setup request

 fs/compat_ioctl.c         |  1 +
 net/bluetooth/bnep/bnep.h |  4 ++++
 net/bluetooth/bnep/core.c | 58 +++++++++++++++++++++++++++++++++++++----------
 net/bluetooth/bnep/sock.c |  8 +++++++
 4 files changed, 59 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH v4 1/4] Bluetooth: bnep: Return err value while sending cmd is not understood

[Grzegorz Kolodziejczyk]

Send command not understood response should be verified if it was
successfully sent, like all send responses.

Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
---
 net/bluetooth/bnep/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index 05f57e4..c627817 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -239,7 +239,7 @@ static int bnep_rx_control(struct bnep_session *s, void *data, int len)
 			pkt[0] = BNEP_CONTROL;
 			pkt[1] = BNEP_CMD_NOT_UNDERSTOOD;
 			pkt[2] = cmd;
-			bnep_send(s, pkt, sizeof(pkt));
+			err = bnep_send(s, pkt, sizeof(pkt));
 		}
 		break;
 	}
-- 
2.1.0

[PATCH v4 2/4] Bluetooth: bnep: Add support for get bnep features via ioctl

[Grzegorz Kolodziejczyk]

This is needed if user space wants to know supported bnep features
by kernel, e.g. if kernel supports sending response to bnep setup
control message. By now there is no possibility to know supported
features by kernel in case of bnep. Ioctls allows only to add connection,
delete connection, get connection list, get connection info. Adding
connection if it's possible (establishing network device connection) is
equivalent to starting bnep session. Bnep session handles data queue of
transmit, receive messages over bnep channel. It means that if we add
connection the received/transmitted data will be parsed immediately. In
case of get bnep features we want to know before session start, if we
should leave setup data on socket queue and let kernel to handle with it,
or in case of no setup handling support, if we should pull this message
and handle setup response within user space.

Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
---
 fs/compat_ioctl.c         | 1 +
 net/bluetooth/bnep/bnep.h | 1 +
 net/bluetooth/bnep/sock.c | 8 ++++++++
 3 files changed, 10 insertions(+)

diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index afec645..19fb0c8 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -570,6 +570,7 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
 #define BNEPCONNDEL	_IOW('B', 201, int)
 #define BNEPGETCONNLIST	_IOR('B', 210, int)
 #define BNEPGETCONNINFO	_IOR('B', 211, int)
+#define BNEPGETSUPPFEAT	_IOR('B', 212, int)
 
 #define CMTPCONNADD	_IOW('C', 200, int)
 #define CMTPCONNDEL	_IOW('C', 201, int)
diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
index 5a5b16f..8709733 100644
--- a/net/bluetooth/bnep/bnep.h
+++ b/net/bluetooth/bnep/bnep.h
@@ -111,6 +111,7 @@ struct bnep_ext_hdr {
 #define BNEPCONNDEL	_IOW('B', 201, int)
 #define BNEPGETCONNLIST	_IOR('B', 210, int)
 #define BNEPGETCONNINFO	_IOR('B', 211, int)
+#define BNEPGETSUPPFEAT	_IOR('B', 212, int)
 
 struct bnep_connadd_req {
 	int   sock;		/* Connected socket */
diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
index 5f05129..f18b6bd 100644
--- a/net/bluetooth/bnep/sock.c
+++ b/net/bluetooth/bnep/sock.c
@@ -58,6 +58,8 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
 	struct socket *nsock;
 	void __user *argp = (void __user *)arg;
 	int err;
+	const __u32 supp_feat = 0;
+
 
 	BT_DBG("cmd %x arg %lx", cmd, arg);
 
@@ -120,6 +122,12 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
 
 		return err;
 
+	case BNEPGETSUPPFEAT:
+		if (copy_to_user(argp, &supp_feat, sizeof(supp_feat)))
+			return -EFAULT;
+
+		return 0;
+
 	default:
 		return -EINVAL;
 	}
-- 
2.1.0

[PATCH v4 3/4] Bluetooth: bnep: Add support to extended headers of control frames

[Grzegorz Kolodziejczyk]

Handling extended headers of control frames is required BNEP
functionality. This patch refractor bnep rx frame handling function.
Extended header for control frames shouldn't be omitted as it was
previously done. Every control frame should be checked if it contains
extended header and then every extension should be parsed separately.

Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
---
 net/bluetooth/bnep/core.c | 46 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 36 insertions(+), 10 deletions(-)

diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index c627817..77ae725 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -292,29 +292,55 @@ static int bnep_rx_frame(struct bnep_session *s, struct sk_buff *skb)
 {
 	struct net_device *dev = s->dev;
 	struct sk_buff *nskb;
-	u8 type;
+	u8 type, ctrl_type;
 
 	dev->stats.rx_bytes += skb->len;
 
 	type = *(u8 *) skb->data;
 	skb_pull(skb, 1);
+	ctrl_type = *(u8 *)skb->data;
 
 	if ((type & BNEP_TYPE_MASK) >= sizeof(__bnep_rx_hlen))
 		goto badframe;
 
 	if ((type & BNEP_TYPE_MASK) == BNEP_CONTROL) {
-		bnep_rx_control(s, skb->data, skb->len);
-		kfree_skb(skb);
-		return 0;
-	}
+		if (bnep_rx_control(s, skb->data, skb->len) < 0) {
+			dev->stats.tx_errors++;
+			kfree_skb(skb);
+			return 0;
+		}
 
-	skb_reset_mac_header(skb);
+		if (!(type & BNEP_EXT_HEADER)) {
+			kfree_skb(skb);
+			return 0;
+		}
 
-	/* Verify and pull out header */
-	if (!skb_pull(skb, __bnep_rx_hlen[type & BNEP_TYPE_MASK]))
-		goto badframe;
+		/* Verify and pull ctrl message since it's already processed */
+		switch (ctrl_type) {
+		case BNEP_SETUP_CONN_REQ:
+			/* Pull: ctrl type (1 b), len (1 b), data (len bytes) */
+			if (!skb_pull(skb, 2 + *(u8 *)(skb->data + 1) * 2))
+				goto badframe;
+			break;
+		case BNEP_FILTER_MULTI_ADDR_SET:
+		case BNEP_FILTER_NET_TYPE_SET:
+			/* Pull: ctrl type (1 b), len (2 b), data (len bytes) */
+			if (!skb_pull(skb, 3 + *(u16 *)(skb->data + 1) * 2))
+				goto badframe;
+			break;
+		default:
+			kfree_skb(skb);
+			return 0;
+		}
+	} else {
+		skb_reset_mac_header(skb);
 
-	s->eh.h_proto = get_unaligned((__be16 *) (skb->data - 2));
+		/* Verify and pull out header */
+		if (!skb_pull(skb, __bnep_rx_hlen[type & BNEP_TYPE_MASK]))
+			goto badframe;
+
+		s->eh.h_proto = get_unaligned((__be16 *) (skb->data - 2));
+	}
 
 	if (type & BNEP_EXT_HEADER) {
 		if (bnep_rx_extension(s, skb) < 0)
-- 
2.1.0

[PATCH v4 4/4] Bluetooth: bnep: Handle BNEP connection setup request

[Grzegorz Kolodziejczyk]

With this patch kernel will be able to handle setup request. This is
needed if we would like to handle control mesages with extension
headers. User space will be only resposible for reading setup data and
checking if scenario is conformance to specification (dst and src device
bnep role). In case of new user space, setup data must be leaved(peek
msg) on queue. New bnep session will be responsible for handling this
data.

Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
---
 net/bluetooth/bnep/bnep.h |  3 +++
 net/bluetooth/bnep/core.c | 10 +++++++++-
 net/bluetooth/bnep/sock.c |  2 +-
 3 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
index 8709733..40854c9 100644
--- a/net/bluetooth/bnep/bnep.h
+++ b/net/bluetooth/bnep/bnep.h
@@ -113,6 +113,9 @@ struct bnep_ext_hdr {
 #define BNEPGETCONNINFO	_IOR('B', 211, int)
 #define BNEPGETSUPPFEAT	_IOR('B', 212, int)
 
+#define BNEP_SETUP_RESPONSE	0
+#define BNEP_SETUP_RSP_SENT	10
+
 struct bnep_connadd_req {
 	int   sock;		/* Connected socket */
 	__u32 flags;
diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
index 77ae725..2135b89 100644
--- a/net/bluetooth/bnep/core.c
+++ b/net/bluetooth/bnep/core.c
@@ -231,7 +231,14 @@ static int bnep_rx_control(struct bnep_session *s, void *data, int len)
 		break;
 
 	case BNEP_SETUP_CONN_REQ:
-		err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP, BNEP_CONN_NOT_ALLOWED);
+		/* Successful response should be sent only once */
+		if (test_bit(BNEP_SETUP_RESPONSE, &s->flags) &&
+		    !test_and_set_bit(BNEP_SETUP_RSP_SENT, &s->flags))
+			err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
+					    BNEP_SUCCESS);
+		else
+			err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
+					    BNEP_CONN_NOT_ALLOWED);
 		break;
 
 	default: {
@@ -592,6 +599,7 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
 	s->sock  = sock;
 	s->role  = req->role;
 	s->state = BT_CONNECTED;
+	s->flags = req->flags & (1 << BNEP_SETUP_RESPONSE);
 
 	s->msg.msg_flags = MSG_NOSIGNAL;
 
diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
index f18b6bd..116e7d3 100644
--- a/net/bluetooth/bnep/sock.c
+++ b/net/bluetooth/bnep/sock.c
@@ -58,7 +58,7 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
 	struct socket *nsock;
 	void __user *argp = (void __user *)arg;
 	int err;
-	const __u32 supp_feat = 0;
+	const __u32 supp_feat = BIT(BNEP_SETUP_RESPONSE);
 
 
 	BT_DBG("cmd %x arg %lx", cmd, arg);
-- 
2.1.0

Re: [PATCH v4 2/4] Bluetooth: bnep: Add support for get bnep features via ioctl

[Marcel Holtmann]

Hi Grzegorz,

> This is needed if user space wants to know supported bnep features
> by kernel, e.g. if kernel supports sending response to bnep setup
> control message. By now there is no possibility to know supported
> features by kernel in case of bnep. Ioctls allows only to add connection,
> delete connection, get connection list, get connection info. Adding
> connection if it's possible (establishing network device connection) is
> equivalent to starting bnep session. Bnep session handles data queue of
> transmit, receive messages over bnep channel. It means that if we add
> connection the received/transmitted data will be parsed immediately. In
> case of get bnep features we want to know before session start, if we
> should leave setup data on socket queue and let kernel to handle with it,
> or in case of no setup handling support, if we should pull this message
> and handle setup response within user space.
> 
> Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
> ---
> fs/compat_ioctl.c         | 1 +
> net/bluetooth/bnep/bnep.h | 1 +
> net/bluetooth/bnep/sock.c | 8 ++++++++
> 3 files changed, 10 insertions(+)
> 
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index afec645..19fb0c8 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -570,6 +570,7 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
> #define BNEPCONNDEL	_IOW('B', 201, int)
> #define BNEPGETCONNLIST	_IOR('B', 210, int)
> #define BNEPGETCONNINFO	_IOR('B', 211, int)
> +#define BNEPGETSUPPFEAT	_IOR('B', 212, int)
> 
> #define CMTPCONNADD	_IOW('C', 200, int)
> #define CMTPCONNDEL	_IOW('C', 201, int)
> diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
> index 5a5b16f..8709733 100644
> --- a/net/bluetooth/bnep/bnep.h
> +++ b/net/bluetooth/bnep/bnep.h
> @@ -111,6 +111,7 @@ struct bnep_ext_hdr {
> #define BNEPCONNDEL	_IOW('B', 201, int)
> #define BNEPGETCONNLIST	_IOR('B', 210, int)
> #define BNEPGETCONNINFO	_IOR('B', 211, int)
> +#define BNEPGETSUPPFEAT	_IOR('B', 212, int)
> 
> struct bnep_connadd_req {
> 	int   sock;		/* Connected socket */
> diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
> index 5f05129..f18b6bd 100644
> --- a/net/bluetooth/bnep/sock.c
> +++ b/net/bluetooth/bnep/sock.c
> @@ -58,6 +58,8 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
> 	struct socket *nsock;
> 	void __user *argp = (void __user *)arg;
> 	int err;
> +	const __u32 supp_feat = 0;

I doubt that const is needed here. It is on the stack anyway. And personally I prefer it before the int err.

> +

This extra empty line is against any coding style. Please be careful with these.

> 	BT_DBG("cmd %x arg %lx", cmd, arg);
> 
> @@ -120,6 +122,12 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
> 
> 		return err;
> 
> +	case BNEPGETSUPPFEAT:
> +		if (copy_to_user(argp, &supp_feat, sizeof(supp_feat)))
> +			return -EFAULT;
> +
> +		return 0;
> +
> 	default:
> 		return -EINVAL;
> 	}

Regards

Marcel

Re: [PATCH v4 4/4] Bluetooth: bnep: Handle BNEP connection setup request

[Marcel Holtmann]

Hi Grzegorz,

> With this patch kernel will be able to handle setup request. This is
> needed if we would like to handle control mesages with extension
> headers. User space will be only resposible for reading setup data and
> checking if scenario is conformance to specification (dst and src device
> bnep role). In case of new user space, setup data must be leaved(peek
> msg) on queue. New bnep session will be responsible for handling this
> data.
> 
> Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
> ---
> net/bluetooth/bnep/bnep.h |  3 +++
> net/bluetooth/bnep/core.c | 10 +++++++++-
> net/bluetooth/bnep/sock.c |  2 +-
> 3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
> index 8709733..40854c9 100644
> --- a/net/bluetooth/bnep/bnep.h
> +++ b/net/bluetooth/bnep/bnep.h
> @@ -113,6 +113,9 @@ struct bnep_ext_hdr {
> #define BNEPGETCONNINFO	_IOR('B', 211, int)
> #define BNEPGETSUPPFEAT	_IOR('B', 212, int)
> 
> +#define BNEP_SETUP_RESPONSE	0
> +#define BNEP_SETUP_RSP_SENT	10
> +
> struct bnep_connadd_req {
> 	int   sock;		/* Connected socket */
> 	__u32 flags;
> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
> index 77ae725..2135b89 100644
> --- a/net/bluetooth/bnep/core.c
> +++ b/net/bluetooth/bnep/core.c
> @@ -231,7 +231,14 @@ static int bnep_rx_control(struct bnep_session *s, void *data, int len)
> 		break;
> 
> 	case BNEP_SETUP_CONN_REQ:
> -		err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP, BNEP_CONN_NOT_ALLOWED);
> +		/* Successful response should be sent only once */
> +		if (test_bit(BNEP_SETUP_RESPONSE, &s->flags) &&
> +		    !test_and_set_bit(BNEP_SETUP_RSP_SENT, &s->flags))
> +			err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
> +					    BNEP_SUCCESS);
> +		else
> +			err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
> +					    BNEP_CONN_NOT_ALLOWED);
> 		break;
> 
> 	default: {
> @@ -592,6 +599,7 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
> 	s->sock  = sock;
> 	s->role  = req->role;
> 	s->state = BT_CONNECTED;
> +	s->flags = req->flags & (1 << BNEP_SETUP_RESPONSE);

Use BIT() here as well.

However this is not really what I asked for. What I asked for was to actually reject any flags that do not have 0 or BIT(0) set. I mean you would just create a mask for this.

> 	s->msg.msg_flags = MSG_NOSIGNAL;
> 
> diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
> index f18b6bd..116e7d3 100644
> --- a/net/bluetooth/bnep/sock.c
> +++ b/net/bluetooth/bnep/sock.c
> @@ -58,7 +58,7 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
> 	struct socket *nsock;
> 	void __user *argp = (void __user *)arg;
> 	int err;
> -	const __u32 supp_feat = 0;
> +	const __u32 supp_feat = BIT(BNEP_SETUP_RESPONSE);
> 
> 
> 	BT_DBG("cmd %x arg %lx", cmd, arg);

Regards

Marcel

Re: [PATCH v4 2/4] Bluetooth: bnep: Add support for get bnep features via ioctl

[Grzegorz Kolodziejczyk]

Hi Marcel,


On 1 April 2015 at 22:26, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Grzegorz,
>
>> This is needed if user space wants to know supported bnep features
>> by kernel, e.g. if kernel supports sending response to bnep setup
>> control message. By now there is no possibility to know supported
>> features by kernel in case of bnep. Ioctls allows only to add connection,
>> delete connection, get connection list, get connection info. Adding
>> connection if it's possible (establishing network device connection) is
>> equivalent to starting bnep session. Bnep session handles data queue of
>> transmit, receive messages over bnep channel. It means that if we add
>> connection the received/transmitted data will be parsed immediately. In
>> case of get bnep features we want to know before session start, if we
>> should leave setup data on socket queue and let kernel to handle with it,
>> or in case of no setup handling support, if we should pull this message
>> and handle setup response within user space.
>>
>> Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
>> ---
>> fs/compat_ioctl.c         | 1 +
>> net/bluetooth/bnep/bnep.h | 1 +
>> net/bluetooth/bnep/sock.c | 8 ++++++++
>> 3 files changed, 10 insertions(+)
>>
>> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
>> index afec645..19fb0c8 100644
>> --- a/fs/compat_ioctl.c
>> +++ b/fs/compat_ioctl.c
>> @@ -570,6 +570,7 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
>> #define BNEPCONNDEL   _IOW('B', 201, int)
>> #define BNEPGETCONNLIST       _IOR('B', 210, int)
>> #define BNEPGETCONNINFO       _IOR('B', 211, int)
>> +#define BNEPGETSUPPFEAT      _IOR('B', 212, int)
>>
>> #define CMTPCONNADD   _IOW('C', 200, int)
>> #define CMTPCONNDEL   _IOW('C', 201, int)
>> diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
>> index 5a5b16f..8709733 100644
>> --- a/net/bluetooth/bnep/bnep.h
>> +++ b/net/bluetooth/bnep/bnep.h
>> @@ -111,6 +111,7 @@ struct bnep_ext_hdr {
>> #define BNEPCONNDEL   _IOW('B', 201, int)
>> #define BNEPGETCONNLIST       _IOR('B', 210, int)
>> #define BNEPGETCONNINFO       _IOR('B', 211, int)
>> +#define BNEPGETSUPPFEAT      _IOR('B', 212, int)
>>
>> struct bnep_connadd_req {
>>       int   sock;             /* Connected socket */
>> diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
>> index 5f05129..f18b6bd 100644
>> --- a/net/bluetooth/bnep/sock.c
>> +++ b/net/bluetooth/bnep/sock.c
>> @@ -58,6 +58,8 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
>>       struct socket *nsock;
>>       void __user *argp = (void __user *)arg;
>>       int err;
>> +     const __u32 supp_feat = 0;
>
> I doubt that const is needed here. It is on the stack anyway. And personally I prefer it before the int err.
>
Ok.
>> +
>
> This extra empty line is against any coding style. Please be careful with these.
>
I've missed it, thanks.
>>       BT_DBG("cmd %x arg %lx", cmd, arg);
>>
>> @@ -120,6 +122,12 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
>>
>>               return err;
>>
>> +     case BNEPGETSUPPFEAT:
>> +             if (copy_to_user(argp, &supp_feat, sizeof(supp_feat)))
>> +                     return -EFAULT;
>> +
>> +             return 0;
>> +
>>       default:
>>               return -EINVAL;
>>       }
>
> Regards
>
> Marcel
>

Regards,
Grzegorz

Re: [PATCH v4 4/4] Bluetooth: bnep: Handle BNEP connection setup request

[Grzegorz Kolodziejczyk]

Hi Marcel,


On 1 April 2015 at 22:28, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Grzegorz,
>
>> With this patch kernel will be able to handle setup request. This is
>> needed if we would like to handle control mesages with extension
>> headers. User space will be only resposible for reading setup data and
>> checking if scenario is conformance to specification (dst and src device
>> bnep role). In case of new user space, setup data must be leaved(peek
>> msg) on queue. New bnep session will be responsible for handling this
>> data.
>>
>> Signed-off-by: Grzegorz Kolodziejczyk <grzegorz.kolodziejczyk@tieto.com>
>> ---
>> net/bluetooth/bnep/bnep.h |  3 +++
>> net/bluetooth/bnep/core.c | 10 +++++++++-
>> net/bluetooth/bnep/sock.c |  2 +-
>> 3 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/bluetooth/bnep/bnep.h b/net/bluetooth/bnep/bnep.h
>> index 8709733..40854c9 100644
>> --- a/net/bluetooth/bnep/bnep.h
>> +++ b/net/bluetooth/bnep/bnep.h
>> @@ -113,6 +113,9 @@ struct bnep_ext_hdr {
>> #define BNEPGETCONNINFO       _IOR('B', 211, int)
>> #define BNEPGETSUPPFEAT       _IOR('B', 212, int)
>>
>> +#define BNEP_SETUP_RESPONSE  0
>> +#define BNEP_SETUP_RSP_SENT  10
>> +
>> struct bnep_connadd_req {
>>       int   sock;             /* Connected socket */
>>       __u32 flags;
>> diff --git a/net/bluetooth/bnep/core.c b/net/bluetooth/bnep/core.c
>> index 77ae725..2135b89 100644
>> --- a/net/bluetooth/bnep/core.c
>> +++ b/net/bluetooth/bnep/core.c
>> @@ -231,7 +231,14 @@ static int bnep_rx_control(struct bnep_session *s, void *data, int len)
>>               break;
>>
>>       case BNEP_SETUP_CONN_REQ:
>> -             err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP, BNEP_CONN_NOT_ALLOWED);
>> +             /* Successful response should be sent only once */
>> +             if (test_bit(BNEP_SETUP_RESPONSE, &s->flags) &&
>> +                 !test_and_set_bit(BNEP_SETUP_RSP_SENT, &s->flags))
>> +                     err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
>> +                                         BNEP_SUCCESS);
>> +             else
>> +                     err = bnep_send_rsp(s, BNEP_SETUP_CONN_RSP,
>> +                                         BNEP_CONN_NOT_ALLOWED);
>>               break;
>>
>>       default: {
>> @@ -592,6 +599,7 @@ int bnep_add_connection(struct bnep_connadd_req *req, struct socket *sock)
>>       s->sock  = sock;
>>       s->role  = req->role;
>>       s->state = BT_CONNECTED;
>> +     s->flags = req->flags & (1 << BNEP_SETUP_RESPONSE);
>
> Use BIT() here as well.
>
> However this is not really what I asked for. What I asked for was to actually reject any flags that do not have 0 or BIT(0) set. I mean you would just create a mask for this.
>
Sorry, I've must misunderstood you. So now I'll rebase on your "mask
bnep flags" patch and set valid_flags to BIT(BNEP_SETUP_RESPONSE) with
session flags set to BIT(BNEP_SETUP_RESPONSE) also - is it ok ?
>>       s->msg.msg_flags = MSG_NOSIGNAL;
>>
>> diff --git a/net/bluetooth/bnep/sock.c b/net/bluetooth/bnep/sock.c
>> index f18b6bd..116e7d3 100644
>> --- a/net/bluetooth/bnep/sock.c
>> +++ b/net/bluetooth/bnep/sock.c
>> @@ -58,7 +58,7 @@ static int bnep_sock_ioctl(struct socket *sock, unsigned int cmd, unsigned long
>>       struct socket *nsock;
>>       void __user *argp = (void __user *)arg;
>>       int err;
>> -     const __u32 supp_feat = 0;
>> +     const __u32 supp_feat = BIT(BNEP_SETUP_RESPONSE);
>>
>>
>>       BT_DBG("cmd %x arg %lx", cmd, arg);
>
> Regards
>
> Marcel
>
Regards,
Grzegorz