Re: [PATCH net-next] net/smc: Introduce TCP ULP support

Re: [PATCH net-next] net/smc: Introduce TCP ULP support

[Al Viro]

	Half a year too late, but then it hadn't been posted on fsdevel.
Which it really should have been, due to

> +	/* replace tcp socket to smc */
> +	smcsock->file = tcp->file;
> +	smcsock->file->private_data = smcsock;
> +	smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */
> +	smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */
> +	tcp->file = NULL;

this.  It violates a bunch of rather fundamental assertions about the
data structures you are playing with, and I'm not even going into the
lifetime and refcounting issues.

	* ->d_inode of a busy positive dentry never changes while refcount
of dentry remains positive.  A lot of places in VFS rely upon that.
	* ->f_inode of a file never changes, period.
	* ->private_data of a struct file associated with a socket never
changes; it can be accessed lockless, with no precautions beyond "make sure
that refcount of struct file will remain positive".

PS: more than one thread could be calling methods of that struct socket at the
same time; what's to stop e.g. connect(2) on the same sucker (e.g. called on
the same descriptor from a different thread that happens to share the same
descriptor table) to be sitting there trying to lock the struct sock currently
held locked by caller of tcp_set_ulp()?

Re: [PATCH net-next] net/smc: Introduce TCP ULP support

[Al Viro]

On Thu, Aug 04, 2022 at 03:56:11AM +0100, Al Viro wrote:
> 	Half a year too late, but then it hadn't been posted on fsdevel.
> Which it really should have been, due to
> 
> > +	/* replace tcp socket to smc */
> > +	smcsock->file = tcp->file;
> > +	smcsock->file->private_data = smcsock;
> > +	smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */
> > +	smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */
> > +	tcp->file = NULL;
> 
> this.  It violates a bunch of rather fundamental assertions about the
> data structures you are playing with, and I'm not even going into the
> lifetime and refcounting issues.
> 
> 	* ->d_inode of a busy positive dentry never changes while refcount
> of dentry remains positive.  A lot of places in VFS rely upon that.
> 	* ->f_inode of a file never changes, period.
> 	* ->private_data of a struct file associated with a socket never
> changes; it can be accessed lockless, with no precautions beyond "make sure
> that refcount of struct file will remain positive".

Consider, BTW, what it does to sockfd_lookup() users.  We grab a reference
to struct file, pick struct socket from its ->private_data, work with that
sucker, then do sockfd_put().  Which does fput(sock->file).

Guess what happens if sockfd_lookup() is given the descriptor of your
TCP socket, just before that tcp->file = NULL?  Right, fput(NULL) as
soon as matching sockfd_put() is called.  And the very first thing fput()
does is this:
        if (atomic_long_dec_and_test(&file->f_count)) {

And that's just one example - a *lot* of places both in VFS and in
net/* rely upon these assertions.  This is really not a workable approach.

Re: [PATCH net-next] net/smc: Introduce TCP ULP support

[Tony Lu]

On Thu, Aug 04, 2022 at 03:56:11AM +0100, Al Viro wrote:
> 	Half a year too late, but then it hadn't been posted on fsdevel.
> Which it really should have been, due to
> 
> > +	/* replace tcp socket to smc */
> > +	smcsock->file = tcp->file;
> > +	smcsock->file->private_data = smcsock;
> > +	smcsock->file->f_inode = SOCK_INODE(smcsock); /* replace inode when sock_close */
> > +	smcsock->file->f_path.dentry->d_inode = SOCK_INODE(smcsock); /* dput() in __fput */
> > +	tcp->file = NULL;
> 
> this.  It violates a bunch of rather fundamental assertions about the
> data structures you are playing with, and I'm not even going into the
> lifetime and refcounting issues.
> 
> 	* ->d_inode of a busy positive dentry never changes while refcount
> of dentry remains positive.  A lot of places in VFS rely upon that.
> 	* ->f_inode of a file never changes, period.
> 	* ->private_data of a struct file associated with a socket never
> changes; it can be accessed lockless, with no precautions beyond "make sure
> that refcount of struct file will remain positive".
>
> PS: more than one thread could be calling methods of that struct socket at the
> same time; what's to stop e.g. connect(2) on the same sucker (e.g. called on
> the same descriptor from a different thread that happens to share the same
> descriptor table) to be sitting there trying to lock the struct sock currently
> held locked by caller of tcp_set_ulp()?

Sorry for the late reply.

SMC ULP tries to make original TCP sockets behave like SMC. The original
TCP sockets will belong to this new SMC socket, and it can only be
accessed in kernel with struct socket in SMC. The SMC and TCP sockets are
bonded together.

So this patch replaces the file of TCP to SMC socket which is allocated
in kernel. It is guaranteed that the TCP socket is always freed before
the newly replaced SMC socket.

There is an other approach to archive this by changing af_ops of sockets.
I will fix it without breaking the assertions.

Tony Lu