I have been experimenting with loadable protocol modules, and ran into
several issues with module reference counting.
The first issue was that __module_get failed at the BUG_ON check at the
top of the routine (checking that my module reference count was not zero)
when I created the first socket. When sk_alloc() is called, my module
reference count was still 0. When I looked at why sctp didn't have this
problem, I discovered that sctp creates a control socket during module
init (when the module ref count is not 0), which keeps the reference
count non-zero. This section has been updated to address the point
Stephen raised about checking the return value of try_module_get().
The next problem arose when my socket init routine returned an error.
This resulted in my module reference count being decremented below 0.
My socket ops->release routine was also being called. The issue here
is that sock_release() calls the ops->release routine and decrements
the ref count if sock->ops is not NULL. Since the socket probably
didn't get correctly initialized, this should not be done, so we will
set sock->ops to NULL because we will not call try_module_get().
While searching for another bug, I also noticed that sys_accept() has a
possibility of doing a module_put() when it did not do an __module_get
so I re-ordered the call to security_socket_accept().
Thanks
Frank
---
Signed-off-by: Frank Filz <[EMAIL PROTECTED]>
diff --git a/net/core/sock.c b/net/core/sock.c
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -660,16 +660,20 @@ struct sock *sk_alloc(int family, unsign
sock_lock_init(sk);
}
- if (security_sk_alloc(sk, family, priority)) {
- if (slab != NULL)
- kmem_cache_free(slab, sk);
- else
- kfree(sk);
- sk = NULL;
- } else
- __module_get(prot->owner);
+ if (security_sk_alloc(sk, family, priority))
+ goto out_free;
+
+ if (!try_module_get(prot->owner))
+ goto out_free;
}
return sk;
+
+out_free:
+ if (slab != NULL)
+ kmem_cache_free(slab, sk);
+ else
+ kfree(sk);
+ return NULL;
}
void sk_free(struct sock *sk)
diff --git a/net/socket.c b/net/socket.c
--- a/net/socket.c
+++ b/net/socket.c
@@ -1145,8 +1145,11 @@ static int __sock_create(int family, int
if (!try_module_get(net_families[family]->owner))
goto out_release;
- if ((err = net_families[family]->create(sock, protocol)) < 0)
+ if ((err = net_families[family]->create(sock, protocol)) < 0) {
+ sock->ops = NULL;
goto out_module_put;
+ }
+
/*
* Now to bump the refcnt of the [loadable] module that owns this
* socket at sock_release time we decrement its refcnt.
@@ -1360,16 +1363,16 @@ asmlinkage long sys_accept(int fd, struc
newsock->type = sock->type;
newsock->ops = sock->ops;
- err = security_socket_accept(sock, newsock);
- if (err)
- goto out_release;
-
/*
* We don't need try_module_get here, as the listening socket (sock)
* has the protocol module (sock->ops->owner) held.
*/
__module_get(newsock->ops->owner);
+ err = security_socket_accept(sock, newsock);
+ if (err)
+ goto out_release;
+
err = sock->ops->accept(sock, newsock, sock->file->f_flags);
if (err < 0)
goto out_release;
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
