j1939: bind() and connect()

j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 973 bytes --]

Hello Kurt,

I've a few questions about the j1939 socket model:

- Should bind() be allowed to any interface, i.e. with can_ifindex == 0?

I don't think so, but it's never checked for this.

- Should a connect() without a previous bind() be allowed?

It's not checked for, but the error handling in sk_connect doesn't take
that into account.

- Should a second bind() be allowed on a previously bind() socket?

As far as I understand the code, there's some support, but when the
interface is switched, j1939_ifindex_stop() is not called.

- Should a second conect() be allowed on a previously connect() socket?

Again some code, but AFAICS no real support.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On 08/14/2017 07:37 PM, Marc Kleine-Budde wrote:
> - Should a second bind() be allowed on a previously bind() socket?
> 
> As far as I understand the code, there's some support, but when the
> interface is switched, j1939_ifindex_stop() is not called.

Ah, there's a check, that you cannot re-bind() to another interface:

> 		if (bound_dev_if != addr->can_ifindex)

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[David Jander]

On Tue, 15 Aug 2017 10:14:18 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/14/2017 07:37 PM, Marc Kleine-Budde wrote:
> > - Should a second bind() be allowed on a previously bind() socket?
> > 
> > As far as I understand the code, there's some support, but when the
> > interface is switched, j1939_ifindex_stop() is not called.  
> 
> Ah, there's a check, that you cannot re-bind() to another interface:
> 
> > 		if (bound_dev_if != addr->can_ifindex)  

This makes sense. re-bind() to another interface only would make sense if
bind() to any interface is also permitted, but that would imply separate
address-claims on all different interfaces with potentially different
source-address being claimed on each one.... don't think the stack should take
care of such complex scenarios, at least not at first IMHO.

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1849 bytes --]

On 08/15/2017 11:01 AM, David Jander wrote:
> On Tue, 15 Aug 2017 10:14:18 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
>> On 08/14/2017 07:37 PM, Marc Kleine-Budde wrote:
>>> - Should a second bind() be allowed on a previously bind() socket?
>>>
>>> As far as I understand the code, there's some support, but when the
>>> interface is switched, j1939_ifindex_stop() is not called.  
>>
>> Ah, there's a check, that you cannot re-bind() to another interface:
>>
>>> 		if (bound_dev_if != addr->can_ifindex)  
> 
> This makes sense. re-bind() to another interface only would make sense if
> bind() to any interface is also permitted, but that would imply separate
> address-claims on all different interfaces with potentially different
> source-address being claimed on each one.... don't think the stack should take
> care of such complex scenarios, at least not at first IMHO.

Bind to any interface doesn't work, you cannot send anything:

> static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> {
[...]
> 	ifindex = jsk->ifindex_started;
> 	if (!ifindex)
> 		return -EBADFD;

And you will probably not receive anything, as the rx-register, which
takes place in j1939_ifindex_start() is not done if you try to bind to
any device (bound_dev_if == 0).

> 	if (bound_dev_if && bound_dev_if != jsk->ifindex_started) {
[...]
> 
> 		ret = j1939_ifindex_start(bound_dev_if);

Then I'll add proper test, that you have to bind to a specific
interface. That will make the code much simpler.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[David Jander]

On Tue, 15 Aug 2017 11:16:44 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/15/2017 11:01 AM, David Jander wrote:
> > On Tue, 15 Aug 2017 10:14:18 +0200
> > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> >   
> >> On 08/14/2017 07:37 PM, Marc Kleine-Budde wrote:  
> >>> - Should a second bind() be allowed on a previously bind() socket?
> >>>
> >>> As far as I understand the code, there's some support, but when the
> >>> interface is switched, j1939_ifindex_stop() is not called.    
> >>
> >> Ah, there's a check, that you cannot re-bind() to another interface:
> >>  
> >>> 		if (bound_dev_if != addr->can_ifindex)    
> > 
> > This makes sense. re-bind() to another interface only would make sense if
> > bind() to any interface is also permitted, but that would imply separate
> > address-claims on all different interfaces with potentially different
> > source-address being claimed on each one.... don't think the stack should take
> > care of such complex scenarios, at least not at first IMHO.  
> 
> Bind to any interface doesn't work, you cannot send anything:
> 
> > static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> > {  
> [...]
> > 	ifindex = jsk->ifindex_started;
> > 	if (!ifindex)
> > 		return -EBADFD;  
> 
> And you will probably not receive anything, as the rx-register, which
> takes place in j1939_ifindex_start() is not done if you try to bind to
> any device (bound_dev_if == 0).
> 
> > 	if (bound_dev_if && bound_dev_if != jsk->ifindex_started) {  
> [...]
> > 
> > 		ret = j1939_ifindex_start(bound_dev_if);  
> 
> Then I'll add proper test, that you have to bind to a specific
> interface. That will make the code much simpler.

I agree. Don't know if the case for bind-any will ever be necessary in the
context of J1939, but we certainly should keep things simple for now.
I've had cases myself where an ECU would talk J1939 on two interfaces at the
same time, but not yet something like a Group-Function server exporting the
same functions on more than one interface. I could think of that use-case
happening though, but it can also be solved easily in user-space opening more
than one socket...

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939: bind() and connect()

[Kurt Van Dijck]

> On Tue, 15 Aug 2017 11:16:44 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
> > On 08/15/2017 11:01 AM, David Jander wrote:
> > > On Tue, 15 Aug 2017 10:14:18 +0200
> > > Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > >   
> > >> On 08/14/2017 07:37 PM, Marc Kleine-Budde wrote:  
> > >>> - Should a second bind() be allowed on a previously bind() socket?
> > >>>
> > >>> As far as I understand the code, there's some support, but when the
> > >>> interface is switched, j1939_ifindex_stop() is not called.    
> > >>
> > >> Ah, there's a check, that you cannot re-bind() to another interface:
> > >>  
> > >>> 		if (bound_dev_if != addr->can_ifindex)    
> > > 
> > > This makes sense. re-bind() to another interface only would make sense if
> > > bind() to any interface is also permitted, but that would imply separate
> > > address-claims on all different interfaces with potentially different
> > > source-address being claimed on each one.... don't think the stack should take
> > > care of such complex scenarios, at least not at first IMHO.  
> > 
> > Bind to any interface doesn't work, you cannot send anything:
> > 
> > > static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> > > {  
> > [...]
> > > 	ifindex = jsk->ifindex_started;
> > > 	if (!ifindex)
> > > 		return -EBADFD;  
> > 
> > And you will probably not receive anything, as the rx-register, which
> > takes place in j1939_ifindex_start() is not done if you try to bind to
> > any device (bound_dev_if == 0).

which is like calling tcpdump when the interface is down, yep.
That's a stupid thing to do, but not necessarily a bad API.

> > 
> > > 	if (bound_dev_if && bound_dev_if != jsk->ifindex_started) {  
> > [...]
> > > 
> > > 		ret = j1939_ifindex_start(bound_dev_if);  
> > 
> > Then I'll add proper test, that you have to bind to a specific
> > interface. That will make the code much simpler.

I agree that in contrast to my initial goal, binding to any interface
complicates things. Since j1939 is not a routed protocol, you should be
aware of at least which interface you're running on.
Userspace programs can still default to can0 :-)

And it probably simplifies the socket code quite much. That's a good
thing too.

> 
> I agree. Don't know if the case for bind-any will ever be necessary in the
> context of J1939, but we certainly should keep things simple for now.
> I've had cases myself where an ECU would talk J1939 on two interfaces at the
> same time, but not yet something like a Group-Function server exporting the
> same functions on more than one interface. I could think of that use-case
> happening though, but it can also be solved easily in user-space opening more
> than one socket...

Indeed, logical ECU's connected to 2+ j1939 busses cannot take any
advantage by combining multiple interfaces. the 2+ busses will always be
addressed independantly, like David suggests by opening more sockets.

Kind regards,
Kurt

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2905 bytes --]

On 08/16/2017 11:08 PM, Kurt Van Dijck wrote:
>>> Bind to any interface doesn't work, you cannot send anything:
>>>
>>>> static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>>>> {  
>>> [...]
>>>> 	ifindex = jsk->ifindex_started;
>>>> 	if (!ifindex)
>>>> 		return -EBADFD;  
>>>
>>> And you will probably not receive anything, as the rx-register, which
>>> takes place in j1939_ifindex_start() is not done if you try to bind to
>>> any device (bound_dev_if == 0).
> 
> which is like calling tcpdump when the interface is down, yep.
> That's a stupid thing to do, but not necessarily a bad API.

From my point of view, you either want to do diagnostics, then you'd use
tcpdump/wireshark. Or you have a legitimate application scenario, but
then you don't want other applications to interfere with the packages
you receive.

First I want to keep it simple and allow only binds to exactly one
interface. If it later turns out, that bind to any is a requirement, we
can still add it.

>>>> 	if (bound_dev_if && bound_dev_if != jsk->ifindex_started) {  
>>> [...]
>>>>
>>>> 		ret = j1939_ifindex_start(bound_dev_if);  
>>>
>>> Then I'll add proper test, that you have to bind to a specific
>>> interface. That will make the code much simpler.
> 
> I agree that in contrast to my initial goal, binding to any interface
> complicates things. Since j1939 is not a routed protocol, you should be
> aware of at least which interface you're running on.
> Userspace programs can still default to can0 :-)
> 
> And it probably simplifies the socket code quite much. That's a good
> thing too.

ACK.

>> I agree. Don't know if the case for bind-any will ever be necessary in the
>> context of J1939, but we certainly should keep things simple for now.
>> I've had cases myself where an ECU would talk J1939 on two interfaces at the
>> same time, but not yet something like a Group-Function server exporting the
>> same functions on more than one interface. I could think of that use-case
>> happening though, but it can also be solved easily in user-space opening more
>> than one socket...
> 
> Indeed, logical ECU's connected to 2+ j1939 busses cannot take any
> advantage by combining multiple interfaces. the 2+ busses will always be
> addressed independantly, like David suggests by opening more sockets.

Let's conclude:
- bind() to any not allowed
- bind() to a single interface needed before {send,rcv}_msg()
- re-bind() to same interface allowed
- bind() before connect() mandatory
- re-connect() allowed

Is this correct?

regards.
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[Kurt Van Dijck]

> Let's conclude:
> - bind() to any not allowed
> - bind() to a single interface needed before {send,rcv}_msg()
> - re-bind() to same interface allowed
> - bind() before connect() mandatory
4x ack

> - re-connect() allowed

As David pointed out, re-connect() is not necessary. If it makes things
much simpler, then drop it for now. right?

> 
> Is this correct?
> 
> regards.
> Marc

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 719 bytes --]

On 08/17/2017 10:28 AM, Kurt Van Dijck wrote:
> 
>> Let's conclude:
>> - bind() to any not allowed
>> - bind() to a single interface needed before {send,rcv}_msg()
>> - re-bind() to same interface allowed
>> - bind() before connect() mandatory
> 4x ack
> 
>> - re-connect() allowed
> 
> As David pointed out, re-connect() is not necessary. If it makes things
> much simpler, then drop it for now. right?

Fine with me.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[Kurt Van Dijck]

> Hello Kurt,
> 
> I've a few questions about the j1939 socket model:
> 
> - Should bind() be allowed to any interface, i.e. with can_ifindex == 0?
> 
> I don't think so, but it's never checked for this.

I'll explain a bit what I tried to accomplish:
* allow bind to any interface, for receive packets only (obviously)
* for rx/tx, only bind to 1 interface.

> 
> - Should a connect() without a previous bind() be allowed?
> 
> It's not checked for, but the error handling in sk_connect doesn't take
> that into account.

Like UDP will auto-bind to the first known local address ...
Then an application would not need to know the local address to use.

> 
> - Should a second bind() be allowed on a previously bind() socket?

Yes, definitely.
This is required to support dynamic addressing without possible losing
relevant messages.

> 
> As far as I understand the code, there's some support, but when the
> interface is switched, j1939_ifindex_stop() is not called.
> 
> - Should a second conect() be allowed on a previously connect() socket?

Never thought on that. This would equally be necessary when the remote
changes SA, but I consider this not a core requirement, since changing
SA implies possible glitches in the upper applications anyway.
closing socket and reopening a new one is allowed in that case.
> 
> Again some code, but AFAICS no real support.
> 
> regards,
> Marc

Kurt

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2777 bytes --]

On 08/16/2017 11:34 AM, Kurt Van Dijck wrote:
>> Hello Kurt,
>>
>> I've a few questions about the j1939 socket model:
>>
>> - Should bind() be allowed to any interface, i.e. with can_ifindex == 0?
>>
>> I don't think so, but it's never checked for this.
> 
> I'll explain a bit what I tried to accomplish:
> * allow bind to any interface, for receive packets only (obviously)
> * for rx/tx, only bind to 1 interface.

Have you tested this? If I understand the code correctly
j1939_ifindex_start() will not be called. So you will only receive
packages from interfaces someone has other socket has bind() to, thus
called j1939_ifindex_start(). And you'll stop receiving packages once
the last socket on that interfaces is closed.

>> - Should a connect() without a previous bind() be allowed?
>>
>> It's not checked for, but the error handling in sk_connect doesn't take
>> that into account.
> 
> Like UDP will auto-bind to the first known local address ...
> Then an application would not need to know the local address to use.

Which will be:

> static int j1939sk_init(struct sock *sk)
> {
[...]
> 	jsk->addr.sa = J1939_NO_ADDR;
> 	jsk->addr.da = J1939_NO_ADDR;
> 	jsk->addr.pgn = J1939_NO_PGN;
[...]
> }

> static int j1939sk_connect(struct socket *sock, struct sockaddr *uaddr,
> 			   int len, int flags)
[...]
> 		j1939_name_local_get(priv, jsk->addr.src);
> 		j1939_addr_local_get(priv, jsk->addr.sa);

right? Does this work, as sendmsg tests:

> static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> {
[...]
> 	if (jsk->addr.sa == J1939_NO_ADDR && !jsk->addr.src)
> 		/* no address assigned yet */
> 		return -EBADFD;
> 

>> - Should a second bind() be allowed on a previously bind() socket?
> 
> Yes, definitely.
> This is required to support dynamic addressing without possible losing
> relevant messages.

With changing the interface or not?

>> As far as I understand the code, there's some support, but when the
>> interface is switched, j1939_ifindex_stop() is not called.
>>
>> - Should a second conect() be allowed on a previously connect() socket?
> 
> Never thought on that. This would equally be necessary when the remote
> changes SA, but I consider this not a core requirement, since changing
> SA implies possible glitches in the upper applications anyway.
> closing socket and reopening a new one is allowed in that case.

So I'll add a check against a re-connect().

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: j1939: bind() and connect()

[David Jander]

On Wed, 16 Aug 2017 12:06:50 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 08/16/2017 11:34 AM, Kurt Van Dijck wrote:
> >> Hello Kurt,
> >>
> >> I've a few questions about the j1939 socket model:
> >>
> >> - Should bind() be allowed to any interface, i.e. with can_ifindex == 0?
> >>
> >> I don't think so, but it's never checked for this.  
> > 
> > I'll explain a bit what I tried to accomplish:
> > * allow bind to any interface, for receive packets only (obviously)
> > * for rx/tx, only bind to 1 interface.  
> 
> Have you tested this? If I understand the code correctly
> j1939_ifindex_start() will not be called. So you will only receive
> packages from interfaces someone has other socket has bind() to, thus
> called j1939_ifindex_start(). And you'll stop receiving packages once
> the last socket on that interfaces is closed.
> 
> >> - Should a connect() without a previous bind() be allowed?
> >>
> >> It's not checked for, but the error handling in sk_connect doesn't take
> >> that into account.  
> > 
> > Like UDP will auto-bind to the first known local address ...
> > Then an application would not need to know the local address to use.  
> 
> Which will be:
> 
> > static int j1939sk_init(struct sock *sk)
> > {  
> [...]
> > 	jsk->addr.sa = J1939_NO_ADDR;
> > 	jsk->addr.da = J1939_NO_ADDR;
> > 	jsk->addr.pgn = J1939_NO_PGN;  
> [...]
> > }  
> 
> > static int j1939sk_connect(struct socket *sock, struct sockaddr *uaddr,
> > 			   int len, int flags)  
> [...]
> > 		j1939_name_local_get(priv, jsk->addr.src);
> > 		j1939_addr_local_get(priv, jsk->addr.sa);  
> 
> right? Does this work, as sendmsg tests:
> 
> > static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> > {  
> [...]
> > 	if (jsk->addr.sa == J1939_NO_ADDR && !jsk->addr.src)
> > 		/* no address assigned yet */
> > 		return -EBADFD;
> >   
> 
> >> - Should a second bind() be allowed on a previously bind() socket?  
> > 
> > Yes, definitely.
> > This is required to support dynamic addressing without possible losing
> > relevant messages.  
> 
> With changing the interface or not?
> 
> >> As far as I understand the code, there's some support, but when the
> >> interface is switched, j1939_ifindex_stop() is not called.
> >>
> >> - Should a second conect() be allowed on a previously connect() socket?  
> > 
> > Never thought on that. This would equally be necessary when the remote
> > changes SA, but I consider this not a core requirement, since changing
> > SA implies possible glitches in the upper applications anyway.
> > closing socket and reopening a new one is allowed in that case.  
> 
> So I'll add a check against a re-connect().

Hmmm... in theory, any node can change SA at any time due to a re-claim. I
always try to make this as transparent as possible to applications. The worst
that I would deem acceptable to happen is that a transport session times-out
and is transparently re-started, or a single message gets lost due to the
inherent race of switching address. Why is a re-connect necessary if one
connects to the NAME instead of the SA?

Best regards,

-- 
David Jander
Protonic Holland.

Re: j1939: bind() and connect()

[Kurt Van Dijck]

> 
> On Wed, 16 Aug 2017 12:06:50 +0200
> Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> 
> > On 08/16/2017 11:34 AM, Kurt Van Dijck wrote:
> > >> Hello Kurt,
> > >>
> > >> I've a few questions about the j1939 socket model:
> > >>
> > >> - Should bind() be allowed to any interface, i.e. with can_ifindex == 0?
> > >>
> > >> I don't think so, but it's never checked for this.  
> > > 
> > > I'll explain a bit what I tried to accomplish:
> > > * allow bind to any interface, for receive packets only (obviously)
> > > * for rx/tx, only bind to 1 interface.  
> > 
> > Have you tested this? If I understand the code correctly
> > j1939_ifindex_start() will not be called. So you will only receive
> > packages from interfaces someone has other socket has bind() to, thus
> > called j1939_ifindex_start(). And you'll stop receiving packages once
> > the last socket on that interfaces is closed.

Well, I'm well aware of the requirement to start j1939 processing on an
interface. I tested this, with that requirement in mind.

As already suggested in another mail in this thread, I don't thread this
like a core requirement. Drop it if it solves things.

> > 
> > >> - Should a connect() without a previous bind() be allowed?
> > >>
> > >> It's not checked for, but the error handling in sk_connect doesn't take
> > >> that into account.  
> > > 
> > > Like UDP will auto-bind to the first known local address ...
> > > Then an application would not need to know the local address to use.  
> > 
> > Which will be:
> > 
> > > static int j1939sk_init(struct sock *sk)
> > > {  
> > [...]
> > > 	jsk->addr.sa = J1939_NO_ADDR;
> > > 	jsk->addr.da = J1939_NO_ADDR;
> > > 	jsk->addr.pgn = J1939_NO_PGN;  
> > [...]
> > > }  
> > 
> > > static int j1939sk_connect(struct socket *sock, struct sockaddr *uaddr,
> > > 			   int len, int flags)  
> > [...]
> > > 		j1939_name_local_get(priv, jsk->addr.src);
> > > 		j1939_addr_local_get(priv, jsk->addr.sa);  
> > 
> > right? Does this work, as sendmsg tests:
> > 
> > > static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> > > {  
> > [...]
> > > 	if (jsk->addr.sa == J1939_NO_ADDR && !jsk->addr.src)
> > > 		/* no address assigned yet */
> > > 		return -EBADFD;
> > >   

Good point!
Consider connect without bind as disfunctional for now.
Maybe, if things clear up, we can try to add it then.

> > 
> > >> - Should a second bind() be allowed on a previously bind() socket?  
> > > 
> > > Yes, definitely.
> > > This is required to support dynamic addressing without possible losing
> > > relevant messages.  
> > 
> > With changing the interface or not?
> > 
> > >> As far as I understand the code, there's some support, but when the
> > >> interface is switched, j1939_ifindex_stop() is not called.

A re-bind should not change interface!
A re-bind is necessary for changing SA or PGN only.

> > >>
> > >> - Should a second conect() be allowed on a previously connect() socket?  
> > > 
> > > Never thought on that. This would equally be necessary when the remote
> > > changes SA, but I consider this not a core requirement, since changing
> > > SA implies possible glitches in the upper applications anyway.
> > > closing socket and reopening a new one is allowed in that case.  
> > 
> > So I'll add a check against a re-connect().
> 
> Hmmm... in theory, any node can change SA at any time due to a re-claim. I
> always try to make this as transparent as possible to applications. The worst
> that I would deem acceptable to happen is that a transport session times-out
> and is transparently re-started, or a single message gets lost due to the
> inherent race of switching address. Why is a re-connect necessary if one
> connects to the NAME instead of the SA?

That's true. A connect to a NAME is SA agnostic (should be), and so
remote address changes are followed transparently.
This is the core reason to put NAME's in kernel.

So a re-connect should not be necessary.

Kind regards,
Kurt

Re: j1939: bind() and connect()

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1899 bytes --]

On 08/16/2017 12:06 PM, Marc Kleine-Budde wrote:

>>> - Should a connect() without a previous bind() be allowed?
>>>
>>> It's not checked for, but the error handling in sk_connect doesn't take
>>> that into account.
>>
>> Like UDP will auto-bind to the first known local address ...
>> Then an application would not need to know the local address to use.
> 
> Which will be:
> 
>> static int j1939sk_init(struct sock *sk)
>> {
> [...]
>> 	jsk->addr.sa = J1939_NO_ADDR;
>> 	jsk->addr.da = J1939_NO_ADDR;
>> 	jsk->addr.pgn = J1939_NO_PGN;
> [...]
>> }
> 
>> static int j1939sk_connect(struct socket *sock, struct sockaddr *uaddr,
>> 			   int len, int flags)
> [...]
>> 		j1939_name_local_get(priv, jsk->addr.src);
>> 		j1939_addr_local_get(priv, jsk->addr.sa);
> 
> right? Does this work, as sendmsg tests:
> 
>> static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
>> {
> [...]
>> 	if (jsk->addr.sa == J1939_NO_ADDR && !jsk->addr.src)
>> 		/* no address assigned yet */
>> 		return -EBADFD;
>>

sendmsg() will fail, as it checks if the socket is in bound state.

> static int j1939sk_sendmsg(struct socket *sock, struct msghdr *msg, size_t size)
> {
> 	struct sock *sk = sock->sk;
> 	struct j1939_sock *jsk = j1939_sk(sk);
> 	struct sockaddr_can *addr = msg->msg_name;
> 	struct j1939_sk_buff_cb *skcb;
> 	struct sk_buff *skb;
> 	struct net_device *dev;
> 	int ifindex;
> 	int ret;
> 
> 	/* various socket state tests */
> 	if (!(jsk->state & J1939_SOCK_BOUND))
> 		return -EBADFD;
> 
Receiving doesn't work, but I don't know why.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]